Message-ID: <62dc3a53-1c2e-aadf-f5b2-2f4ad75a201a@denx.de>
Date:   Fri, 18 Jan 2019 07:29:35 +0100
From:   Marek Vasut <marex@...x.de>
To:     Tristram.Ha@...rochip.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Pavel Machek <pavel@....cz>,
        Dan Carpenter <dan.carpenter@...cle.com>
Cc:     vivien.didelot@...oirfairelinux.com, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH RFC 1/4] net: dsa: microchip: convert KSZ9477 SPI driver
 to use regmap

On 1/17/19 4:22 AM, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Convert KSZ9477 SPI driver to use regmap mechanism so that an I2C driver
> can be easily added.
> 
> KSZ9477 SPI driver uses a 32-bit SPI command containing a 24-bit address
> to access registers in 8-bit.  The address is automatically increased to
> next register.  In theory all registers can be read in one transfer as
> long as the buffer is enough.  Therefore the regmap_raw_read and
> regmap_raw_write functions are used for basic 8-bit access.  Two more
> regmap configurations are used for 16-bit and 32-bit accesses just that
> regmap_update_bits can be used.
> 
> All variables and functions associated with SPI access are removed.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>

These extremely patches look similar to what I posted here before:

https://patchwork.ozlabs.org/cover/1017222/

But the authorship has changed. Why ?

> ---
>  drivers/net/dsa/microchip/Kconfig       |   1 +
>  drivers/net/dsa/microchip/ksz9477_spi.c | 131 ++++++++++----------------------
>  drivers/net/dsa/microchip/ksz_common.c  |   9 +--
>  drivers/net/dsa/microchip/ksz_common.h  | 113 ++++++++-------------------
>  drivers/net/dsa/microchip/ksz_priv.h    |  28 +------
>  5 files changed, 80 insertions(+), 202 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> index bea29fd..385b93f 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -12,5 +12,6 @@ menuconfig NET_DSA_MICROCHIP_KSZ9477
>  config NET_DSA_MICROCHIP_KSZ9477_SPI
>  	tristate "KSZ9477 series SPI connected switch driver"
>  	depends on NET_DSA_MICROCHIP_KSZ9477 && SPI
> +	select REGMAP_SPI
>  	help
>  	  Select to enable support for registering switches configured through SPI.
> diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
> index d757ba1..d5f0aaa 100644
> --- a/drivers/net/dsa/microchip/ksz9477_spi.c
> +++ b/drivers/net/dsa/microchip/ksz9477_spi.c
> @@ -2,18 +2,14 @@
>  /*
>   * Microchip KSZ9477 series register access through SPI
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
> -#include <asm/unaligned.h>
> -
> -#include <linux/delay.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/spi/spi.h>
>  
>  #include "ksz_priv.h"
> -#include "ksz_spi.h"
>  
>  /* SPI frame opcodes */
>  #define KS_SPIOP_RD			3
> @@ -23,106 +19,61 @@
>  #define SPI_ADDR_MASK			(BIT(SPI_ADDR_SHIFT) - 1)
>  #define SPI_TURNAROUND_SHIFT		5
>  
> -/* Enough to read all switch port registers. */
> -#define SPI_TX_BUF_LEN			0x100
> -
> -static int ksz9477_spi_read_reg(struct spi_device *spi, u32 reg, u8 *val,
> -				unsigned int len)
> -{
> -	u32 txbuf;
> -	int ret;
> -
> -	txbuf = reg & SPI_ADDR_MASK;
> -	txbuf |= KS_SPIOP_RD << SPI_ADDR_SHIFT;
> -	txbuf <<= SPI_TURNAROUND_SHIFT;
> -	txbuf = cpu_to_be32(txbuf);
> -
> -	ret = spi_write_then_read(spi, &txbuf, 4, val, len);
> -	return ret;
> -}
> -
> -static int ksz9477_spi_write_reg(struct spi_device *spi, u32 reg, u8 *val,
> -				 unsigned int len)
> -{
> -	u32 *txbuf = (u32 *)val;
> -
> -	*txbuf = reg & SPI_ADDR_MASK;
> -	*txbuf |= (KS_SPIOP_WR << SPI_ADDR_SHIFT);
> -	*txbuf <<= SPI_TURNAROUND_SHIFT;
> -	*txbuf = cpu_to_be32(*txbuf);
> -
> -	return spi_write(spi, txbuf, 4 + len);
> -}
> -
> -static int ksz_spi_read(struct ksz_device *dev, u32 reg, u8 *data,
> -			unsigned int len)
> -{
> -	struct spi_device *spi = dev->priv;
> -
> -	return ksz9477_spi_read_reg(spi, reg, data, len);
> -}
> -
> -static int ksz_spi_write(struct ksz_device *dev, u32 reg, void *data,
> -			 unsigned int len)
> -{
> -	struct spi_device *spi = dev->priv;
> -
> -	if (len > SPI_TX_BUF_LEN)
> -		len = SPI_TX_BUF_LEN;
> -	memcpy(&dev->txbuf[4], data, len);
> -	return ksz9477_spi_write_reg(spi, reg, dev->txbuf, len);
> -}
> -
> -static int ksz_spi_read24(struct ksz_device *dev, u32 reg, u32 *val)
> -{
> -	int ret;
> -
> -	*val = 0;
> -	ret = ksz_spi_read(dev, reg, (u8 *)val, 3);
> -	if (!ret) {
> -		*val = be32_to_cpu(*val);
> -		/* convert to 24bit */
> -		*val >>= 8;
> -	}
> -
> -	return ret;
> -}
> -
> -static int ksz_spi_write24(struct ksz_device *dev, u32 reg, u32 value)
> -{
> -	/* make it to big endian 24bit from MSB */
> -	value <<= 8;
> -	value = cpu_to_be32(value);
> -	return ksz_spi_write(dev, reg, &value, 3);
> +#define SPI_CMD_LEN			4
> +#define REG_SIZE			0x8000
> +
> +#define SPI_REGMAP_PAD			SPI_TURNAROUND_SHIFT
> +#define SPI_REGMAP_VAL			8
> +#define SPI_REGMAP_REG			\
> +	(SPI_CMD_LEN * SPI_REGMAP_VAL - SPI_TURNAROUND_SHIFT)
> +#define SPI_REGMAP_MASK_S		\
> +	(SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT - \
> +	(SPI_CMD_LEN * SPI_REGMAP_VAL - 8))
> +
> +#define KSZ_REGMAP_COMMON(n, width)					\
> +{									\
> +	.name			= n,					\
> +	.max_register		= REG_SIZE - (width),			\
> +	.reg_bits		= SPI_REGMAP_REG,			\
> +	.val_bits		= SPI_REGMAP_VAL * (width),		\
> +	.pad_bits		= SPI_REGMAP_PAD,			\
> +	.reg_stride		= (width),				\
> +	.read_flag_mask		= KS_SPIOP_RD << SPI_REGMAP_MASK_S,	\
> +	.write_flag_mask	= KS_SPIOP_WR << SPI_REGMAP_MASK_S,	\
> +	.reg_format_endian	= REGMAP_ENDIAN_BIG,			\
> +	.val_format_endian	= REGMAP_ENDIAN_BIG,			\
>  }
>  
> -static const struct ksz_io_ops ksz9477_spi_ops = {
> -	.read8 = ksz_spi_read8,
> -	.read16 = ksz_spi_read16,
> -	.read24 = ksz_spi_read24,
> -	.read32 = ksz_spi_read32,
> -	.write8 = ksz_spi_write8,
> -	.write16 = ksz_spi_write16,
> -	.write24 = ksz_spi_write24,
> -	.write32 = ksz_spi_write32,
> -	.get = ksz_spi_get,
> -	.set = ksz_spi_set,
> +static const struct regmap_config ksz9477_regmap_cfg[] = {
> +	KSZ_REGMAP_COMMON("8", 1),
> +	KSZ_REGMAP_COMMON("16", 2),
> +	KSZ_REGMAP_COMMON("32", 4),
>  };
>  
>  static int ksz9477_spi_probe(struct spi_device *spi)
>  {
>  	struct ksz_device *dev;
> +	int i;
>  	int ret;
>  
> -	dev = ksz_switch_alloc(&spi->dev, &ksz9477_spi_ops, spi);
> +	dev = ksz_switch_alloc(&spi->dev);
>  	if (!dev)
>  		return -ENOMEM;
>  
> +	for (i = 0; i < ARRAY_SIZE(ksz9477_regmap_cfg); i++) {
> +		dev->regmap[i] = devm_regmap_init_spi(spi,
> +						      &ksz9477_regmap_cfg[i]);
> +		if (IS_ERR(dev->regmap[i])) {
> +			ret = PTR_ERR(dev->regmap[i]);
> +			dev_err(&spi->dev, "Failed to initialize regmap: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
>  	if (spi->dev.platform_data)
>  		dev->pdata = spi->dev.platform_data;
>  
> -	dev->txbuf = devm_kzalloc(dev->dev, 4 + SPI_TX_BUF_LEN, GFP_KERNEL);
> -
>  	ret = ksz9477_switch_register(dev);
>  
>  	/* Main DSA driver may not be started yet. */
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 8a5111f..d50a194 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip switch driver main logic
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
>  #include <linux/delay.h>
> @@ -260,9 +260,7 @@ void ksz_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
>  }
>  EXPORT_SYMBOL_GPL(ksz_disable_port);
>  
> -struct ksz_device *ksz_switch_alloc(struct device *base,
> -				    const struct ksz_io_ops *ops,
> -				    void *priv)
> +struct ksz_device *ksz_switch_alloc(struct device *base)
>  {
>  	struct dsa_switch *ds;
>  	struct ksz_device *swdev;
> @@ -279,8 +277,6 @@ struct ksz_device *ksz_switch_alloc(struct device *base,
>  	swdev->dev = base;
>  
>  	swdev->ds = ds;
> -	swdev->priv = priv;
> -	swdev->ops = ops;
>  
>  	return swdev;
>  }
> @@ -305,7 +301,6 @@ int ksz_switch_register(struct ksz_device *dev,
>  		gpiod_set_value(dev->reset_gpio, 0);
>  	}
>  
> -	mutex_init(&dev->reg_mutex);
>  	mutex_init(&dev->stats_mutex);
>  	mutex_init(&dev->alu_mutex);
>  	mutex_init(&dev->vlan_mutex);
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 2dd832d..2dfab32 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0
>   * Microchip switch driver common header
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
>  #ifndef __KSZ_COMMON_H
> @@ -38,20 +38,18 @@ static inline int ksz_read8(struct ksz_device *dev, u32 reg, u8 *val)
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->read8(dev, reg, val);
> -	mutex_unlock(&dev->reg_mutex);
> +	ret = regmap_raw_read(dev->regmap[0], reg, val, 1);
>  
>  	return ret;
>  }
>  
>  static inline int ksz_read16(struct ksz_device *dev, u32 reg, u16 *val)
>  {
> +	unsigned int data;
>  	int ret;
>  
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->read16(dev, reg, val);
> -	mutex_unlock(&dev->reg_mutex);
> +	ret = regmap_read(dev->regmap[1], reg, &data);
> +	*val = (u16)data;
>  
>  	return ret;
>  }
> @@ -60,9 +58,12 @@ static inline int ksz_read24(struct ksz_device *dev, u32 reg, u32 *val)
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->read24(dev, reg, val);
> -	mutex_unlock(&dev->reg_mutex);
> +	ret = regmap_raw_read(dev->regmap[0], reg, val, 3);
> +	if (!ret) {
> +		*val = be32_to_cpu(*val);
> +		/* convert to 24bit */
> +		*val >>= 8;
> +	}
>  
>  	return ret;
>  }
> @@ -71,144 +72,94 @@ static inline int ksz_read32(struct ksz_device *dev, u32 reg, u32 *val)
>  {
>  	int ret;
>  
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->read32(dev, reg, val);
> -	mutex_unlock(&dev->reg_mutex);
> +	ret = regmap_read(dev->regmap[2], reg, val);
>  
>  	return ret;
>  }
>  
>  static inline int ksz_write8(struct ksz_device *dev, u32 reg, u8 value)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->write8(dev, reg, value);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	return regmap_raw_write(dev->regmap[0], reg, &value, 1);
>  }
>  
>  static inline int ksz_write16(struct ksz_device *dev, u32 reg, u16 value)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->write16(dev, reg, value);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	value = cpu_to_be16(value);
> +	return regmap_raw_write(dev->regmap[0], reg, &value, 2);
>  }
>  
>  static inline int ksz_write24(struct ksz_device *dev, u32 reg, u32 value)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->write24(dev, reg, value);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	/* make it to big endian 24bit from MSB */
> +	value <<= 8;
> +	value = cpu_to_be32(value);
> +	return regmap_raw_write(dev->regmap[0], reg, &value, 3);
>  }
>  
>  static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->write32(dev, reg, value);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	value = cpu_to_be32(value);
> +	return regmap_raw_write(dev->regmap[0], reg, &value, 4);
>  }
>  
>  static inline int ksz_get(struct ksz_device *dev, u32 reg, void *data,
>  			  size_t len)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->get(dev, reg, data, len);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	return regmap_raw_read(dev->regmap[0], reg, data, len);
>  }
>  
>  static inline int ksz_set(struct ksz_device *dev, u32 reg, void *data,
>  			  size_t len)
>  {
> -	int ret;
> -
> -	mutex_lock(&dev->reg_mutex);
> -	ret = dev->ops->set(dev, reg, data, len);
> -	mutex_unlock(&dev->reg_mutex);
> -
> -	return ret;
> +	return regmap_raw_write(dev->regmap[0], reg, data, len);
>  }
>  
>  static inline void ksz_pread8(struct ksz_device *dev, int port, int offset,
>  			      u8 *data)
>  {
> -	ksz_read8(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_read8(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static inline void ksz_pread16(struct ksz_device *dev, int port, int offset,
>  			       u16 *data)
>  {
> -	ksz_read16(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_read16(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static inline void ksz_pread32(struct ksz_device *dev, int port, int offset,
>  			       u32 *data)
>  {
> -	ksz_read32(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_read32(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static inline void ksz_pwrite8(struct ksz_device *dev, int port, int offset,
>  			       u8 data)
>  {
> -	ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_write8(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static inline void ksz_pwrite16(struct ksz_device *dev, int port, int offset,
>  				u16 data)
>  {
> -	ksz_write16(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_write16(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
>  				u32 data)
>  {
> -	ksz_write32(dev, dev->dev_ops->get_port_addr(port, offset), data);
> +	ksz_write32(dev, PORT_CTRL_ADDR(port, offset), data);
>  }
>  
>  static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
>  {
> -	u8 data;
> -
> -	ksz_read8(dev, addr, &data);
> -	if (set)
> -		data |= bits;
> -	else
> -		data &= ~bits;
> -	ksz_write8(dev, addr, data);
> +	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
>  }
>  
>  static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
>  			 bool set)
>  {
> -	u32 addr;
> -	u8 data;
> -
> -	addr = dev->dev_ops->get_port_addr(port, offset);
> -	ksz_read8(dev, addr, &data);
> -
> -	if (set)
> -		data |= bits;
> -	else
> -		data &= ~bits;
> -
> -	ksz_write8(dev, addr, data);
> +	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset), bits,
> +			   set ? bits : 0);
>  }
>  
>  #endif
> diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
> index 60b4901..526cd0f 100644
> --- a/drivers/net/dsa/microchip/ksz_priv.h
> +++ b/drivers/net/dsa/microchip/ksz_priv.h
> @@ -2,7 +2,7 @@
>   *
>   * Microchip KSZ series switch common definitions
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2019 Microchip Technology Inc.
>   */
>  
>  #ifndef __KSZ_PRIV_H
> @@ -11,13 +11,12 @@
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
>  #include <linux/phy.h>
> +#include <linux/regmap.h>
>  #include <linux/etherdevice.h>
>  #include <net/dsa.h>
>  
>  #include "ksz9477_reg.h"
>  
> -struct ksz_io_ops;
> -
>  struct vlan_table {
>  	u32 table[3];
>  };
> @@ -47,18 +46,15 @@ struct ksz_device {
>  	struct dsa_switch *ds;
>  	struct ksz_platform_data *pdata;
>  	const char *name;
> +	struct regmap *regmap[3];
>  
> -	struct mutex reg_mutex;		/* register access */
>  	struct mutex stats_mutex;	/* status access */
>  	struct mutex alu_mutex;		/* ALU access */
>  	struct mutex vlan_mutex;	/* vlan access */
> -	const struct ksz_io_ops *ops;
>  	const struct ksz_dev_ops *dev_ops;
>  
>  	struct device *dev;
>  
> -	void *priv;
> -
>  	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
>  
>  	/* chip specific data */
> @@ -81,8 +77,6 @@ struct ksz_device {
>  
>  	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
>  
> -	u8 *txbuf;
> -
>  	struct ksz_port *ports;
>  	struct timer_list mib_read_timer;
>  	struct work_struct mib_read;
> @@ -101,19 +95,6 @@ struct ksz_device {
>  	u16 port_mask;
>  };
>  
> -struct ksz_io_ops {
> -	int (*read8)(struct ksz_device *dev, u32 reg, u8 *value);
> -	int (*read16)(struct ksz_device *dev, u32 reg, u16 *value);
> -	int (*read24)(struct ksz_device *dev, u32 reg, u32 *value);
> -	int (*read32)(struct ksz_device *dev, u32 reg, u32 *value);
> -	int (*write8)(struct ksz_device *dev, u32 reg, u8 value);
> -	int (*write16)(struct ksz_device *dev, u32 reg, u16 value);
> -	int (*write24)(struct ksz_device *dev, u32 reg, u32 value);
> -	int (*write32)(struct ksz_device *dev, u32 reg, u32 value);
> -	int (*get)(struct ksz_device *dev, u32 reg, void *data, size_t len);
> -	int (*set)(struct ksz_device *dev, u32 reg, void *data, size_t len);
> -};
> -
>  struct alu_struct {
>  	/* entry 1 */
>  	u8	is_static:1;
> @@ -158,8 +139,7 @@ struct ksz_dev_ops {
>  	void (*exit)(struct ksz_device *dev);
>  };
>  
> -struct ksz_device *ksz_switch_alloc(struct device *base,
> -				    const struct ksz_io_ops *ops, void *priv);
> +struct ksz_device *ksz_switch_alloc(struct device *base);
>  int ksz_switch_register(struct ksz_device *dev,
>  			const struct ksz_dev_ops *ops);
>  void ksz_switch_remove(struct ksz_device *dev);
> 


-- 
Best regards,
Marek Vasut

Powered by blists - more mailing lists