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
