Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 25 Jan 2022 16:50:53 +0200 | Subject | Re: [PATCH v13, 2/2] net: Add dm9051 driver |
| |
On Tue, Jan 25, 2022 at 10:59 AM Joseph CHAMG <josright123@gmail.com> 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@kernel.org> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Leon Romanovsky <leon@kernel.org> > Cc: andy Shevchenko <andy.shevchenko@gmail.com>
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
| |