lkml.org 
[lkml]   [2022]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2022-01-25 16:02    [W:0.082 / U:1.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site