Messages in this thread | | | Date | Fri, 24 Dec 2021 19:36:59 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH v8, 2/2] net: Add dm9051 driver |
| |
> +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
| |