On Tue, Jan 25, 2022 at 10:59 AM Joseph CHAMG <josright123@xxxxxxxxx> wrote:
>
> Add davicom dm9051 spi ethernet driver. The driver work for the
> device platform with spi master
This is better, but please use a grammar period as well.
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: Andrew Lunn <andrew@xxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Andy
And you may utilize --cc parameter to git send-email or move this Cc
block behind the cutter '--- ' line.
...
> +#include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/phy.h>
> +#include <linux/regmap.h>
> +#include <linux/skbuff.h>
> +#include <linux/spinlock.h>
> +#include <linux/spi/spi.h>
types.h is missing
...
> +/**
> + * struct rx_ctl_mach - rx activities record
> + *
In all similar cases remove this blank line.
> + * @status_err_counter: rx status error counter
> + * @large_err_counter: rx get large packet length error counter
> + * @fifo_rst_counter: reset operation counter
> + *
> + * To keep track for the driver operation statistics
> + */
...
> +/**
> + * struct dm9051_rxhdr - rx packet data header
> + *
> + * @rxpktready: lead byte is 0x01 tell a valid packet
> + * @rxstatus: status bits for the received packet
> + * @rxlen: packet length
> + *
> + * The rx packet enter into the fifo memory is start with these four
The Rx packed, entered into the FIFO memory, starts with
> + * bytes which is the rx header, follow this header is the ethernet
Rx header, followed by the ethernet
> + * packet data and end with a appended 4-byte CRC data.
ends
an appended
> + * Both rx packet and CRC data are for check purpose and finally are
> + * dropped by this driver
> + */
...
> + * @kw_rxctrl: kernel thread worke structure for rx control
worker?
...
> + int ret = regmap_read_poll_timeout(db->regmap_dm, DM9051_NSR, mval,
> + mval & (NSR_TX2END | NSR_TX1END), 1, 20);
> +
> + if (ret)
Please, split the assignment and get it closer to its user, so
int ret;
ret = ...
if (ret)
This applies to all similar cases.
Actually all comments are against the entire code even if it's given
only for one occurrence of the similar code block.
> + netdev_err(db->ndev, "timeout in checking for tx ends\n");
> + return ret;
> +}
...
> + ret = regmap_bulk_read(db->regmap_dmbulk, DM9051_EPDRL, to, 2);
> + if (ret < 0)
> + return ret;
> + return ret;
return regmap_...(...);
Same for other similar places.
...
> + /* this is a 2 bytes data written via regmap_bulk_write
> + */
Useless comments.
...
> +static int dm9051_mdiobus_read(struct mii_bus *mdiobus, int phy_id, int reg)
> +{
> + struct board_info *db = mdiobus->priv;
> + unsigned int val = 0;
You can do
val = 0xffff;
here...
> + int ret;
> +
> + if (phy_id == DM9051_PHY_ID) {
> + ret = ctrl_dm9051_phyread(db, reg, &val);
> + if (ret)
> + return ret;
> + return val;
> + }
> + return 0xffff;
...and
}
return val;
here.
> +}
> + while (count--) {
If the count is guaranteed to be greater than 0, it would be better to use
do {
...
} while (--count);
> + ret = regmap_read(db->regmap_dm, reg, &rb);
> + if (ret) {
> + netif_err(db, drv, ndev, "%s: error %d dumping read reg %02x\n",
> + __func__, ret, reg);
> + break;
> + }
> + }
> + return ret;
> +}
...
> +#ifndef _DM9051_H_
> +#define _DM9051_H_
> +
> +#include <linux/bitfield.h>
There is no user of this header, but missing bits.h and one that
provides netdev_priv().
...
> +#define DRVNAME_9051 "dm9051"
Why is this in the header?
--
With Best Regards,
Andy Shevchenko
