Hi Simon, On Thu, Feb 12, 2015 at 11:20 PM, Simon Glass <s...@chromium.org> wrote: > > Hi Joe, > > On 10 February 2015 at 23:25, Joe Hershberger <joe.hershber...@gmail.com> wrote: > > > > > > On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass <s...@chromium.org> wrote: > >> > >> Hi Joe, > >> > >> On 2 February 2015 at 17:38, Joe Hershberger <joe.hershber...@ni.com> > >> wrote: > >> > First just add support for MAC drivers. > >> > > >> > Signed-off-by: Joe Hershberger <joe.hershber...@ni.com> > >> > > >> > --- > >> > > >> > Changes in v2: > >> > -Updated comments > >> > -Removed extra parentheses > >> > -Changed eth_uclass_priv local var names to be uc_priv > >> > -Update error codes > >> > -Cause an invalid name to fail binding > >> > -Rebase on top of dm/master > >> > -Stop maintaining our own index and use DM seq now that it works for our > >> > needs > >> > -Move the hwaddr to platdata so that its memory is allocated at bind > >> > when we need it > >> > -Prevent device from being probed before used by a command (i.e. before > >> > eth_init()). > >> > > >> > common/board_r.c | 4 +- > >> > common/cmd_bdinfo.c | 2 + > >> > include/dm/uclass-id.h | 1 + > >> > include/net.h | 24 ++++ > >> > net/eth.c | 319 > >> > ++++++++++++++++++++++++++++++++++++++++++++++++- > >> > 5 files changed, 346 insertions(+), 4 deletions(-) > >> > > > [snip] > > >> > diff --git a/net/eth.c b/net/eth.c > >> > index c02548c..1b5a169 100644 > >> > --- a/net/eth.c > >> > +++ b/net/eth.c > >> > @@ -72,6 +72,320 @@ static int eth_mac_skip(int index) > >> > return ((skip_state = getenv(enetvar)) != NULL); > >> > } > >> > > >> > +static void eth_current_changed(void); > >> > + > >> > +#ifdef CONFIG_DM_ETH > >> > +#include <dm.h> > >> > +#include <dm/device-internal.h> > >> > >> These should go at the top of the file - should be OK to always > >> include them (i.e. no #ifdef) > > > > OK... Moved them. > > > >> > + > >> > +struct eth_device_priv { > >> > + int state; > >> > + void *priv; > >> > +}; > >> > + > >> > +struct eth_uclass_priv { > >> > + struct udevice *current; > >> > +}; > >> > + > >> > +static void eth_set_current_to_next(void) > >> > +{ > >> > + struct uclass *uc; > >> > + struct eth_uclass_priv *uc_priv; > >> > + > >> > + uclass_get(UCLASS_ETH, &uc); > >> > >> This can actually return an error, although I agree there is little > >> point in handling it. So I suppose this is OK. > > > > I think this should be a given. > > > >> > + uc_priv = uc->priv; > >> > + uclass_next_device(&uc_priv->current); > >> > + if (!uc_priv->current) > >> > + uclass_first_device(UCLASS_ETH, &uc_priv->current); > >> > +} > >> > + > >> > +struct udevice *eth_get_dev(void) > >> > +{ > >> > + struct uclass *uc; > >> > >> blank line here > > > > OK on all of these. > > > > > >> > + uclass_get(UCLASS_ETH, &uc); > >> > + > >> > + struct eth_uclass_priv *uc_priv = uc->priv; > >> > + return uc_priv->current; > >> > +} > >> > + > >> > +static void eth_set_dev(struct udevice *dev) > >> > +{ > >> > + struct uclass *uc; > >> > >> blank line here > >> > >> > + uclass_get(UCLASS_ETH, &uc); > >> > + > >> > + struct eth_uclass_priv *uc_priv = uc->priv; > >> > + uc_priv->current = dev; > >> > +} > >> > + > >> > +unsigned char *eth_get_ethaddr(void) > >> > +{ > >> > + struct eth_pdata *pdata; > >> > >> blank line here > >> > >> > + if (eth_get_dev()) { > >> > + pdata = eth_get_dev()->platdata; > >> > + if (pdata) > >> > + return pdata->enetaddr; > >> > + } > >> > + return NULL; > >> > +} > >> > + > >> > +/* Set active state */ > >> > +int eth_init_state_only(bd_t *bis) > >> > +{ > >> > + struct eth_device_priv *priv; > >> > >> blank line here > >> > >> > + if (eth_get_dev()) { > >> > + priv = eth_get_dev()->uclass_priv; > >> > + if (priv) > >> > + priv->state = ETH_STATE_ACTIVE; > >> > + } > >> > + > >> > + return 0; > >> > +} > >> > +/* Set passive state */ > >> > +void eth_halt_state_only(void) > >> > +{ > >> > + struct eth_device_priv *priv; > >> > >> blank line here > >> > >> > + if (eth_get_dev()) { > >> > + priv = eth_get_dev()->uclass_priv; > >> > + if (priv) > >> > + priv->state = ETH_STATE_PASSIVE; > >> > + } > >> > +} > >> > + > >> > +int eth_get_dev_index(void) > >> > +{ > >> > + if (eth_get_dev()) > >> > + return eth_get_dev()->seq; > >> > + return -1; > >> > +} > >> > + > >> > +int eth_init(bd_t *bis) > >> > +{ > >> > + struct udevice *current, *old_current, *dev; > >> > + struct uclass *uc; > >> > + > >> > + current = eth_get_dev(); > >> > + if (!current) { > >> > + puts("No ethernet found.\n"); > >> > + return -1; > >> > + } > >> > + device_probe(current); > >> > >> Check error > > > > OK. > > > >> > + > >> > + /* Sync environment with network devices */ > >> > + uclass_get(UCLASS_ETH, &uc); > >> > + uclass_foreach_dev(dev, uc) { > >> > + uchar env_enetaddr[6]; > >> > + > >> > + if (eth_getenv_enetaddr_by_index("eth", dev->seq, > >> > + env_enetaddr)) { > >> > + struct eth_pdata *pdata = dev->platdata; > >> > >> blank line > >> > >> Do all devices have the same platdata by design? What if a particular > >> device wants its own? > > > > That's a good question. I imagine some devices may have something unique, > > but I would expect they would read that data into the priv data that they > > define. How do other subsystems handle platform data for unique devices? > > There isn't great support for it. Typically the device has its own > platform data. For buses there is per-child platform data, which the > uclass can define, but we don't have uclass-defined platform data for > normal devices (which are not children).
So I guess that means we should leave it until needed and then extend core to support such things on non-bus-children? > > > >> > + if (pdata) > >> > >> What do you need this check? > > > > No. This was left over from when enetaddr was in priv. > > > > > >> > + memcpy(pdata->enetaddr, env_enetaddr, > >> > 6); > >> > + } > >> > + }; > >> > + > >> > + old_current = current; > >> > + do { > >> > + debug("Trying %s\n", current->name); > >> > + > >> > + if (current->driver) { > >> > + const struct eth_ops *ops = > >> > current->driver->ops; > >> > >> blank line > >> > >> > + if (ops->init(current, bis) >= 0) { > >> > + struct eth_device_priv *priv = > >> > + current->uclass_priv; > >> > + if (priv) > >> > + priv->state = ETH_STATE_ACTIVE; > >> > + > >> > + return 0; > >> > + } > >> > + } > >> > + debug("FAIL\n"); > >> > + > >> > + eth_try_another(0); > >> > + current = eth_get_dev(); > >> > + } while (old_current != current); > >> > + > >> > + return -ENODEV; > >> > +} > >> > + > >> > +void eth_halt(void) > >> > +{ > >> > + struct udevice *current; > >> > + > >> > + current = eth_get_dev(); > >> > + if (!current) > >> > + return; > >> > + if (!current->driver) > >> > + return; > >> > + > >> > + const struct eth_ops *ops = current->driver->ops; > >> > + ops->halt(current); > >> > + > >> > + struct eth_device_priv *priv = current->uclass_priv; > >> > + if (priv) > >> > + priv->state = ETH_STATE_PASSIVE; > >> > +} > >> > + > >> > +int eth_send(void *packet, int length) > >> > +{ > >> > + struct udevice *current; > >> > + > >> > + current = eth_get_dev(); > >> > + if (!current) > >> > + return -1; > >> > + if (!current->driver) > >> > + return -1; > >> > >> Seems like eth_get_dev() should return an error code if current or > >> current->driver is NULL. > > > > It's returning a pointer, so I prefer it to be NULL or a valid pointer. > > > > Perhaps you meant to say that eth_send() should return an error code? > > Well yes that too. But -1 is not a valid error anymore. You need to > use -EPERM or something. And whatever went wrong with eth_get_dev() > should really be returned. So you could do: > > ret = eth_get_dev(¤t); > if (ret) > return ret; I don't see this as equivalent. > or if you prefer: > > current = eth_get_dev(); > if (IS_ERR(current)) > return PTR_ERR(current); This makes more sense, but given that eth_get_dev() is not doing anything other than returning a static global variable, it seems that "NULL" is a valid return. That's just the value of that variable. Nothing "went wrong". It seems like it's jumping through hoops to convert NULL into some error code, return that code as a pointer, then convert it back, when the test for NULL is far more straightforward in the caller and easier to read. I completely agree about not returning -1 from eth_send() and already have that change in for the next version. Thanks, -Joe _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot