> +config DM9051
> + tristate "DM9051 SPI support"
> + select PHYLIB
> + depends on SPI
> + select CRC32
> + select MII
Since you are using PHYLIB, you should not require MII.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/interrupt.h>
> +#include <linux/phy.h>
> +#include <linux/skbuff.h>
> +#include <linux/spinlock.h>
> +#include <linux/cache.h>
> +#include <linux/crc32.h>
> +#include <linux/mii.h>
> +#include <linux/ethtool.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/iopoll.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +
> +#include "dm9051.h"
> +
> +static int msg_enable = NETIF_MSG_PROBE |
> + NETIF_MSG_LINK |
> + NETIF_MSG_IFDOWN |
> + NETIF_MSG_IFUP |
> + NETIF_MSG_RX_ERR |
> + NETIF_MSG_TX_ERR;
> +
> +module_param(msg_enable, int, 0);
> +MODULE_PARM_DESC(msg_enable, "Message mask bitmapped");
No module parameters please. You have the need ethtool ops to control
this.
> +
> +/* spi low level code */
> +static inline int dm9051_xfer(struct board_info *db, u8 cmd, u8 *txb, u8 *rxb, unsigned int len)
No inline functions in C code, let the compiler decide.
> +{
> + struct device *dev = &db->spidev->dev;
> + int ret;
> +
> + db->cmd[0] = cmd;
> + db->spi_xfer2[0].tx_buf = &db->cmd[0];
> + db->spi_xfer2[0].rx_buf = NULL;
> + db->spi_xfer2[0].len = 1;
> + if (!rxb) {
> + db->spi_xfer2[1].tx_buf = txb;
> + db->spi_xfer2[1].rx_buf = NULL;
> + db->spi_xfer2[1].len = len;
> + } else {
> + db->spi_xfer2[1].tx_buf = txb;
> + db->spi_xfer2[1].rx_buf = rxb;
> + db->spi_xfer2[1].len = len;
> + }
Nit pick, but i would invert the logic here, to make it more natural.
> + ret = spi_sync(db->spidev, &db->spi_msg);
> + if (ret < 0)
> + dev_err(dev, "spi burst cmd 0x%02x, ret=%d\n", cmd, ret);
> + return ret;
> +}
> +
> +static u8 dm9051_ior(struct board_info *db, unsigned int reg)
> +{
> + int ret;
> + u8 rxb[1];
> +
> + ret = dm9051_xfer(db, DM_SPI_RD | reg, NULL, rxb, 1);
> + if (ret < 0)
> + return 0xff;
No, return the error code which dm9051_xfer() returns.
> + return rxb[0];
> +}
> +
> +static void dm9051_iow(struct board_info *db, unsigned int reg, unsigned int val)
> +{
> + u8 txb[1];
> +
> + txb[0] = val;
> + dm9051_xfer(db, DM_SPI_WR | reg, txb, NULL, 1);
This should be an int function, which returns 0, or the error code
which dm9051_xfer() returns.
> +}
> +
> +static int dm9051_inblk(struct board_info *db, u8 *buff, unsigned int len)
> +{
> + int ret;
> + u8 txb[1];
> +
> + ret = dm9051_xfer(db, DM_SPI_RD | DM_SPI_MRCMD, txb, buff, len);
> + return ret;
> +}
> +
> +static void dm9051_dumpblk(struct board_info *db, unsigned int len)
> +{
> + while (len--)
> + dm9051_ior(db, DM_SPI_MRCMD);
and here you need to look for an error from dm9051_ior() and return it
to the caller etc.
> +static int dm9051_phy_read(struct board_info *db, int reg, int *pvalue)
> +{
> + int ret;
> + u8 check_val;
> +
> + dm9051_iow(db, DM9051_EPAR, DM9051_PHY | reg);
> + dm9051_iow(db, DM9051_EPCR, EPCR_ERPRR | EPCR_EPOS);
> + ret = read_poll_timeout(dm9051_ior, check_val, !(check_val & EPCR_ERRE), 100, 10000,
> + true, db, DM9051_EPCR);
> + dm9051_iow(db, DM9051_EPCR, 0x0);
> + if (ret) {
> + netdev_err(db->ndev, "timeout read phy register\n");
> + return -ETIMEDOUT;
> + }
You should check for an error before doing the dm9051_iow(). And don't
return -ETIMEDOUT, return whatever read_poll_timeout() returns as an
error code.
Your error handling in this driver needs a lot of work. Please improve
it for the next submission. Error codes need returning as far as
possible up the call stack.
Andrew
