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).
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;
or if you prefer:
current = eth_get_dev(); if (IS_ERR(current)) return PTR_ERR(current);
const struct eth_ops *ops = current->driver->ops;return ops->send(current, packet, length);+}
Regards, Simon