[RFC,1/4] net: dsa: microchip: convert KSZ9477 SPI driver to use regmap
diff mbox series

Message ID 1547695326-31553-2-git-send-email-Tristram.Ha@microchip.com
State RFC
Delegated to: David Miller
Headers show
Series
  • Convert KSZ9477 SPI driver to use regmap
Related show

Commit Message

Tristram.Ha@microchip.com Jan. 17, 2019, 3:22 a.m. UTC
From: Tristram Ha <Tristram.Ha@microchip.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@microchip.com>
---
 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(-)

Comments

Marek Vasut Jan. 18, 2019, 6:29 a.m. UTC | #1
On 1/17/19 4:22 AM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.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@microchip.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);
>
Pavel Machek Jan. 18, 2019, 10:50 a.m. UTC | #2
On Fri 2019-01-18 07:29:35, Marek Vasut wrote:
> On 1/17/19 4:22 AM, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <Tristram.Ha@microchip.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@microchip.com>
> 
> These extremely patches look similar to what I posted here before:
> 
> https://patchwork.ozlabs.org/cover/1017222/
> 
> But the authorship has changed. Why ?

There seems to be explanation in 0/4.

Tristram: if you started from Marek's patches as you describe in 0/4,
you should mark respective patches as From: and Signed-off-by:
Marek...

Best regards,
								Pavel
Tristram.Ha@microchip.com Jan. 18, 2019, 8:32 p.m. UTC | #3
> > These extremely patches look similar to what I posted here before:
> >
> > https://patchwork.ozlabs.org/cover/1017222/
> >
> > But the authorship has changed. Why ?
> 
> There seems to be explanation in 0/4.
> 
> Tristram: if you started from Marek's patches as you describe in 0/4,
> you should mark respective patches as From: and Signed-off-by:
> Marek...

Sorry, do not know much about pulling out the patches and applying them and there are several versions of Marek's patchset.

This is a RFC so please suggest more changes.
Marek Vasut Jan. 19, 2019, 6:23 a.m. UTC | #4
On 1/18/19 9:32 PM, Tristram.Ha@microchip.com wrote:
>>> These extremely patches look similar to what I posted here before:
>>>
>>> https://patchwork.ozlabs.org/cover/1017222/
>>>
>>> But the authorship has changed. Why ?
>>
>> There seems to be explanation in 0/4.
>>
>> Tristram: if you started from Marek's patches as you describe in 0/4,
>> you should mark respective patches as From: and Signed-off-by:
>> Marek...
> 
> Sorry, do not know much about pulling out the patches and applying them and there are several versions of Marek's patchset.
> 
> This is a RFC so please suggest more changes.

My suggestion is that you take the RFC V2 patchset I posted, adjust it
so it works with the KSZ9477 and post those changes. Or post V3 of that
patchset, but make sure to retain authorship.
Pavel Machek Jan. 19, 2019, 1:22 p.m. UTC | #5
On Fri 2019-01-18 20:32:17, Tristram.Ha@microchip.com wrote:
> > > These extremely patches look similar to what I posted here before:
> > >
> > > https://patchwork.ozlabs.org/cover/1017222/
> > >
> > > But the authorship has changed. Why ?
> > 
> > There seems to be explanation in 0/4.
> > 
> > Tristram: if you started from Marek's patches as you describe in 0/4,
> > you should mark respective patches as From: and Signed-off-by:
> > Marek...
> 
> Sorry, do not know much about pulling out the patches and applying them and there are several versions of Marek's patchset.
> 

For patches that are originally from Marek, you should have:

From: Marek Vasut <marex@denx.de>

and

Signed-off-by: Marek Vasut <marex@denx.de>

That is important even for RFC patches. See
Documentation/process/submitting-patches.rst  for details.

Best regards,
								Pavel

Patch
diff mbox series

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);