Re: [PATCH v9 18/37] net: eth-uclass: add function eth_start_udev()

Hi Jerome,

On Fri, 23 Aug 2024 at 07:49, Jerome Forissier jerome.forissier@linaro.org wrote:

Add a function to start a given network device, and update eth_init() to use it.

Signed-off-by: Jerome Forissier jerome.forissier@linaro.org Reviewed-by: Tom Rini trini@konsulko.com

---

drivers/mtd/altera_qspi.c | 4 ++-- include/net-common.h | 1 + net/eth-uclass.c | 38 +++++++++++++++++++++++++------------- 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c index c26615821c8..e5c8df750b7 100644 --- a/drivers/mtd/altera_qspi.c +++ b/drivers/mtd/altera_qspi.c @@ -96,7 +96,7 @@ int flash_erase(flash_info_t *info, int s_first, int s_last) ret = mtd_erase(mtd, &instr); flash_set_verbose(0); if (ret)

•           return ERR_PROTECTED;
  

•           return FL_ERR_PROTECTED;
  

Could you put these in another patch?

    puts(" done\n");
    return 0;

@@ -114,7 +114,7 @@ int write_buff(flash_info_t *info, uchar *src, ulong addr, ulong cnt)

    ret = mtd_write(mtd, to, cnt, &retlen, src);
    if (ret)

•           return ERR_PROTECTED;
  

•           return FL_ERR_PROTECTED;
  
    return 0;
  

} diff --git a/include/net-common.h b/include/net-common.h index 26674ec7e90..43bc6b9a0d5 100644 --- a/include/net-common.h +++ b/include/net-common.h @@ -192,6 +192,7 @@ int eth_env_set_enetaddr_by_index(const char *base_name, int index, int usb_ether_init(void);

int eth_init(void); /* Initialize the device */ +int eth_start_udev(struct udevice *dev);

Please add a proper function comment to all exported functions.

int eth_send(void *packet, int length); /* Send a packet */ #if defined(CONFIG_API) || defined(CONFIG_EFI_LOADER) int eth_receive(void *packet, int length); /* Receive a packet*/ diff --git a/net/eth-uclass.c b/net/eth-uclass.c index e34d7af0229..6dd9b9bb98e 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -284,6 +284,27 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op, } U_BOOT_ENV_CALLBACK(ethaddr, on_ethaddr);

+int eth_start_udev(struct udevice *dev) +{

•   struct eth_device_priv *priv = dev_get_uclass_priv(dev);
  

•   int ret;
  

•   if (priv->running)
  

•           return 0;
  

•   if (!device_active(dev))
  

•           return -EINVAL;
  

•   ret = eth_get_ops(dev)->start(dev);
  

•   if (ret >= 0) {
  

It is better to have the error path jump out, so:

if (ret < 0) return ret;

•           priv->state = ETH_STATE_ACTIVE;
  

•           priv->running = true;
  

•           ret = 0;
  

•   }
  

•   return ret;
  

return 0

+}

int eth_init(void) { struct udevice *current = NULL; @@ -328,20 +349,11 @@ int eth_init(void) if (current) { debug("Trying %s\n", current->name);

•                   if (device_active(current)) {
  

•                           ret = eth_get_ops(current)->start(current);
  

•                           if (ret >= 0) {
  

•                                   struct eth_device_priv *priv =
  

•                                           dev_get_uclass_priv(current);
  

•                                   priv->state = ETH_STATE_ACTIVE;
  

•                                   priv->running = true;
  

•                                   ret = 0;
  

•                                   goto end;
  

•                           }
  

•                   } else {
  

•                   ret = eth_start_udev(current);
  

•                   if (ret < 0)
                            ret = eth_errno;
  

•                   }
  

•                   else
  

•                           goto end;
  

Can that be just a 'break' ?

                    debug("FAIL\n");
            } else {

-- 2.40.1

Regards, Simon