Re: [PATCH v9, 2/2] net: Add dm9051 driver

[Andrew Lunn <andrew@lunn.ch>]

> +#define	dm9051_inb(db, reg, prxb)	dm9051_xfer(db, DM_SPI_RD | (reg), NULL, prxb, 1)
> +#define	dm9051_outb(db, reg, ptxb)	dm9051_xfer(db, DM_SPI_WR | (reg), ptxb, NULL, 1)
> +#define	dm9051_inblk(db, buff, len)	dm9051_xfer(db, DM_SPI_RD | DM_SPI_MRCMD, NULL, buff, len)
> +#define	dm9051_outblk(db, buff, len)	dm9051_xfer(db, DM_SPI_WR | DM_SPI_MWCMD, buff, NULL, len)

How many times do i have to say it, no wrappers. This makes it a lot
less obvious you are throwing away the error code from dm9051_xfer().

> +/* spi low level code */
> +static int dm9051_xfer(struct board_info *db, u8 cmd, u8 *txb, u8 *rxb, unsigned int len)
> +{
> +	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;
> +	db->spi_xfer2[1].tx_buf = txb;
> +	db->spi_xfer2[1].rx_buf = rxb;
> +	db->spi_xfer2[1].len = len;
> +	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_getreg(struct board_info *db, unsigned int reg)
> +{
> +	int ret;
> +	u8 rxb[1];
> +
> +	ret = dm9051_inb(db, reg, rxb);
> +	if (ret < 0) {

If ret < 0 you probably have had an SPI error, -EIO, or maybe -ETIMEDOUT? 

> +		if (reg == DM9051_NSR)
> +			return NSR_TX2END | NSR_TX1END;
> +		if (reg == DM9051_EPCR)
> +			return (u8)(~EPCR_ERRE);

So please add a comment explaining why it is O.K. to return these
values when you probably cannot even talk to the device.

> +		return 0;

And here you should be returning the error code. And whatever called
dm9051_getreg() needs to deal with that error code, probably returning
it to its caller, etc.

> +	}
> +	return rxb[0];
> +}
> +

> +static int dm9051_phy_read(struct board_info *db, int reg, int *pvalue)
> +{
> +	int ret;
> +	u8 check_val;
> +	u8 eph, epl;
> +
> +	dm9051_iow(db, DM9051_EPAR, DM9051_PHY | reg);

dm9051_iow() returns an error code. You should check for an error, and
return it. dm9051_mdio_read() already does the correct thing, passing
the error code along. The MDIO core will then pass the error along to
phylib, and phylib will probably report the error to user space and
probably disable the PHY, since the PHY is no longer accessible. That
is how error handling should work.

Since you still do not have the trivial things correct, i've not done
a deep review yet. I would not expect this driver to be merged very
soon, and until somebody does make a deeper review, there could be
more serious things wrong with this driver, like locking.

     Andrew