Hi Simon,
On Thu, Feb 12, 2015 at 11:20 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 10 February 2015 at 23:25, Joe Hershberger joe.hershberger@gmail.com
wrote:
On Fri, Feb 6, 2015 at 7:25 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 2 February 2015 at 17:38, Joe Hershberger joe.hershberger@ni.com wrote:
First just add support for MAC drivers.
Signed-off-by: Joe Hershberger joe.hershberger@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