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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jan 5, 2022 at 10:18 AM Joseph CHAMG <josright123@gmail.com> wrote:

Missed commit message.

> v1-v4
>
> Add davicom dm9051 spi ethernet driver. The driver work for the
> device platform with spi master
>
> Test ok with raspberry pi 2 and pi 4, the spi configure used in
> my raspberry pi 4 is spi0.1, spi speed 31200000, and INT by pin 26.
>
> v5
>
> Work to eliminate the wrappers to be clear for read, swapped to
> phylib for phy connection tasks.
>
> Tested with raspberry pi 4. Test for netwroking function, CAT5
> cable unplug/plug and also ethtool detect for link state, and
> all are ok.
>
> v6
>
> remove the redundant code that phylib has support,
> adjust to be the reasonable sequence,
> fine tune comments, add comments for pause function support
>
> Tested with raspberry pi 4. Test for netwroking function, CAT5
> cable unplug/plug and also ethtool detect for link state, and
> all are ok.
>
> v7
>
> read/write registers must return error code to the callet,
> add to enable pause processing
>
> v8
>
> not parmanently set MAC by .ndo_set_mac_address
>
> correct rx function such as clear ISR,
> inblk avoid stack buffer,
> simple skb buffer process and
> easy use netif_rx_ni.
>
> simplely queue init and wake the queues,
> limit the start_xmit function use netif_stop_queue.
>
> descript that schedule delay is essential
> for tx_work and rxctrl_work
>
> eliminate ____cacheline_aligned and
> add static int msg_enable.
>
> v9
>
> use phylib, no need 'select MII' in Kconfig,
> make it clear in dm9051_xfer when using spi_sync,
> improve the registers read/write so that error code
> return as far as possible up the call stack.
>
> v10
>
> use regmap APIs for SPI and MDIO,
> modify to correcting such as include header files
> and program check styles

Changelog should go after the cutter '--- ' line below...

> 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>
> Signed-off-by: Joseph CHAMG <josright123@gmail.com>
> ---

...somewhere here.

...

> +#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>

...

> +/* spi master low level code */
> +static int hw_dm9051_spi_write(void *context, u8 reg, const u8 *data, size_t count)

> +static int hw_dm9051_spi_read(void *context, u8 reg, u8 *data, size_t count)

> +static int regmap_dm9051_reg_write(void *context, const void *data, size_t len)

> +static int regmap_dm9051_reg_read(void *context, const void *reg_buf, size_t reg_size,
> +                                 void *val, size_t val_size)

> +static int regmap_dm9051_reg_update_bits(void *context, unsigned int reg,
> +                                        unsigned int mask,
> +                                        unsigned int val)

All these are implemented by regmap SPI API. Why don't you use it?

...

> +static bool dm9051_regmap_readable(struct device *dev, unsigned int reg)
> +{
> +       return true;
> +}
> +
> +static bool dm9051_regmap_writeable(struct device *dev, unsigned int reg)
> +{
> +       return true;
> +}
> +
> +static bool dm9051_regmap_volatile(struct device *dev, unsigned int reg)
> +{
> +       return true; /* optional false */
> +}
> +
> +static bool dm9051_regmap_precious(struct device *dev, unsigned int reg)
> +{
> +       return true; /* optional false */
> +}

These stubs are redundant.

...

> +static void regmap_lock_mutex(void *context)
> +{
> +       struct board_info *db = context;
> +
> +       mutex_lock(&db->regmap_mutex);
> +}
> +
> +static void regmap_unlock_mutex(void *context)
> +{
> +       struct board_info *db = context;
> +
> +       mutex_unlock(&db->regmap_mutex);
> +}

Why do you need this? Either you use lock provided by regmap, or you
disable locking for regmap and provide your own locking scheme (should
be justified in the commit message).

...

> +static struct regmap_config regconfig = {

> +       .name = "reg",

Do you need this?

> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = 0xff,
> +       .reg_stride = 1,
> +       .cache_type = REGCACHE_RBTREE,
> +       .val_format_endian = REGMAP_ENDIAN_LITTLE,

> +};

...

> +static int dm9051_map_poll(struct board_info *db)
> +{
> +       unsigned int mval;
> +       int ret;
> +
> +       ret = read_poll_timeout(regmap_read, ret, !ret || !(mval & EPCR_ERRE),
> +                               100, 10000, true, db->regmap, DM9051_EPCR, &mval);
> +       if (ret)
> +               netdev_err(db->ndev, "timeout in processing for phy/eeprom accessing\n");
> +       return ret;
> +}

regmap has a corresponding API, i.e. regmap_read_poll_timeout().

...

> +static int regmap_dm9051_phy_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +       struct board_info *db = context;
> +       int ret;
> +
> +       regmap_write(db->regmap, DM9051_EPAR, DM9051_PHY | reg);

> +       regmap_write(db->regmap, DM9051_EPDRL, val & 0xff);
> +       regmap_write(db->regmap, DM9051_EPDRH, (val >> 8) && 0xff);

Shouldn't be this a bulk write?
Ditto for all other cases where you need to write 16-bit values in
sequential addresses.

> +       regmap_write(db->regmap, DM9051_EPCR, EPCR_EPOS | EPCR_ERPRW);
> +       ret = dm9051_map_poll(db);
> +       regmap_write(db->regmap, DM9051_EPCR, 0x0);
> +
> +       if (reg == MII_BMCR && !(val & 0x0800))
> +               mdelay(1); /* need for if activate phyxcer */
> +
> +       return ret;
> +}

...

> +static int devm_regmap_init_dm9051(struct device *dev, struct board_info *db)
> +{
> +       mutex_init(&db->regmap_mutex);
> +
> +       regconfig.lock_arg = db;
> +
> +       db->regmap = devm_regmap_init(dev, &regmap_spi, db, &regconfig);
> +       if (IS_ERR(db->regmap))
> +               return PTR_ERR(db->regmap);
> +       db->phymap = devm_regmap_init(dev, &phymap_mdio, db, &phyconfig);
> +       if (IS_ERR(db->phymap))
> +               return PTR_ERR(db->phymap);

Use corresponding regmap APIs, i.e. MDIO, SPI, etc.

> +       return 0;
> +}

...

> +{
> +       int ret;
> +       u8 rxb[1];
> +
> +       while (len--) {
> +               ret = hw_dm9051_spi_read(db, DM_SPI_MRCMD, rxb, 1);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +       return ret;
> +}

I believe the regmap API provides this kind of read (bulk with no
increment addresses or so).

I stopped here, because it's enough for now. Just take your time to
see how other (recent) drivers are implemented.

-- 
With Best Regards,
Andy Shevchenko