[PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Sven Van Asbroeck]

v1:
	starting point.
	is there a way to test-run this on supported devices that I don't
		have physical access to - (ks8995, ksz8864) ?

Sven Van Asbroeck (1):
  spi_ks8995: use regmap to access chip registers.

 drivers/net/phy/spi_ks8995.c | 163 +++++++++++++------------------------------
 1 file changed, 50 insertions(+), 113 deletions(-)

-- 
1.9.1

[PATCH v1 1/1] spi_ks8995: use regmap to access chip registers.

[Sven Van Asbroeck]

The register map layouts used in this driver are well suited to
being accessed through a regmap. This makes the driver simpler
and shorter, by eliminating some spi boilerplate code.

Testing:
- tested on a ksz8785.
- not tested on the other supported chips (ks8995, ksz8864)
  because I don't have access to them.
  However, I instrumented the spi layer to verify that the
  correct spi transactions are generated to read the ID
  registers on those chips.

Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>
---
 drivers/net/phy/spi_ks8995.c | 163 +++++++++++++------------------------------
 1 file changed, 50 insertions(+), 113 deletions(-)

diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c
index 1e2d4f1..34223ff 100644
--- a/drivers/net/phy/spi_ks8995.c
+++ b/drivers/net/phy/spi_ks8995.c
@@ -21,6 +21,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/regmap.h>
 
 #include <linux/spi/spi.h>
 
@@ -106,11 +107,27 @@ enum ks8995_chip_variant {
 
 struct ks8995_chip_params {
 	char *name;
+	const struct regmap_config *regmap_cfg;
 	int family_id;
 	int chip_id;
 	int regs_size;
-	int addr_width;
-	int addr_shift;
+};
+
+static const struct regmap_config ksz8795_regmap_cfg = {
+	.reg_bits = 15,
+	.pad_bits = 1,
+	.val_bits = 8,
+	.write_flag_mask = KS8995_CMD_WRITE << 5,
+	.read_flag_mask = KS8995_CMD_READ << 5,
+	/* max_register filled in at runtime */
+};
+
+static const struct regmap_config ks8995_regmap_cfg = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.write_flag_mask = KS8995_CMD_WRITE,
+	.read_flag_mask = KS8995_CMD_READ,
+	/* max_register filled in at runtime */
 };
 
 static const struct ks8995_chip_params ks8995_chip[] = {
@@ -119,24 +136,21 @@ struct ks8995_chip_params {
 		.family_id = FAMILY_KS8995,
 		.chip_id = KS8995_CHIP_ID,
 		.regs_size = KS8995_REGS_SIZE,
-		.addr_width = 8,
-		.addr_shift = 0,
+		.regmap_cfg = &ks8995_regmap_cfg,
 	},
 	[ksz8864] = {
 		.name = "KSZ8864RMN",
 		.family_id = FAMILY_KS8995,
 		.chip_id = KSZ8864_CHIP_ID,
 		.regs_size = KSZ8864_REGS_SIZE,
-		.addr_width = 8,
-		.addr_shift = 0,
+		.regmap_cfg = &ks8995_regmap_cfg,
 	},
 	[ksz8795] = {
 		.name = "KSZ8795CLX",
 		.family_id = FAMILY_KSZ8795,
 		.chip_id = KSZ8795_CHIP_ID,
 		.regs_size = KSZ8795_REGS_SIZE,
-		.addr_width = 12,
-		.addr_shift = 1,
+		.regmap_cfg = &ksz8795_regmap_cfg,
 	},
 };
 
@@ -152,6 +166,7 @@ struct ks8995_switch {
 	struct bin_attribute	regs_attr;
 	const struct ks8995_chip_params	*chip;
 	int			revision_id;
+	struct regmap *regmap;
 };
 
 static const struct spi_device_id ks8995_id[] = {
@@ -162,118 +177,24 @@ struct ks8995_switch {
 };
 MODULE_DEVICE_TABLE(spi, ks8995_id);
 
-static inline u8 get_chip_id(u8 val)
+static inline u8 get_chip_id(u32 val)
 {
 	return (val >> ID1_CHIPID_S) & ID1_CHIPID_M;
 }
 
-static inline u8 get_chip_rev(u8 val)
+static inline u8 get_chip_rev(u32 val)
 {
 	return (val >> ID1_REVISION_S) & ID1_REVISION_M;
 }
 
-/* create_spi_cmd - create a chip specific SPI command header
- * @ks: pointer to switch instance
- * @cmd: SPI command for switch
- * @address: register address for command
- *
- * Different chip families use different bit pattern to address the switches
- * registers:
- *
- * KS8995: 8bit command + 8bit address
- * KSZ8795: 3bit command + 12bit address + 1bit TR (?)
- */
-static inline __be16 create_spi_cmd(struct ks8995_switch *ks, int cmd,
-				    unsigned address)
-{
-	u16 result = cmd;
-
-	/* make room for address (incl. address shift) */
-	result <<= ks->chip->addr_width + ks->chip->addr_shift;
-	/* add address */
-	result |= address << ks->chip->addr_shift;
-	/* SPI protocol needs big endian */
-	return cpu_to_be16(result);
-}
-/* ------------------------------------------------------------------------ */
-static int ks8995_read(struct ks8995_switch *ks, char *buf,
-		 unsigned offset, size_t count)
-{
-	__be16 cmd;
-	struct spi_transfer t[2];
-	struct spi_message m;
-	int err;
-
-	cmd = create_spi_cmd(ks, KS8995_CMD_READ, offset);
-	spi_message_init(&m);
-
-	memset(&t, 0, sizeof(t));
-
-	t[0].tx_buf = &cmd;
-	t[0].len = sizeof(cmd);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].rx_buf = buf;
-	t[1].len = count;
-	spi_message_add_tail(&t[1], &m);
-
-	mutex_lock(&ks->lock);
-	err = spi_sync(ks->spi, &m);
-	mutex_unlock(&ks->lock);
-
-	return err ? err : count;
-}
-
-static int ks8995_write(struct ks8995_switch *ks, char *buf,
-		 unsigned offset, size_t count)
-{
-	__be16 cmd;
-	struct spi_transfer t[2];
-	struct spi_message m;
-	int err;
-
-	cmd = create_spi_cmd(ks, KS8995_CMD_WRITE, offset);
-	spi_message_init(&m);
-
-	memset(&t, 0, sizeof(t));
-
-	t[0].tx_buf = &cmd;
-	t[0].len = sizeof(cmd);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].tx_buf = buf;
-	t[1].len = count;
-	spi_message_add_tail(&t[1], &m);
-
-	mutex_lock(&ks->lock);
-	err = spi_sync(ks->spi, &m);
-	mutex_unlock(&ks->lock);
-
-	return err ? err : count;
-}
-
-static inline int ks8995_read_reg(struct ks8995_switch *ks, u8 addr, u8 *buf)
-{
-	return ks8995_read(ks, buf, addr, 1) != 1;
-}
-
-static inline int ks8995_write_reg(struct ks8995_switch *ks, u8 addr, u8 val)
-{
-	char buf = val;
-
-	return ks8995_write(ks, &buf, addr, 1) != 1;
-}
-
-/* ------------------------------------------------------------------------ */
-
 static int ks8995_stop(struct ks8995_switch *ks)
 {
-	return ks8995_write_reg(ks, KS8995_REG_ID1, 0);
+	return regmap_write(ks->regmap, KS8995_REG_ID1, 0);
 }
 
 static int ks8995_start(struct ks8995_switch *ks)
 {
-	return ks8995_write_reg(ks, KS8995_REG_ID1, 1);
+	return regmap_write(ks->regmap, KS8995_REG_ID1, 1);
 }
 
 static int ks8995_reset(struct ks8995_switch *ks)
@@ -294,11 +215,13 @@ static ssize_t ks8995_registers_read(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev;
 	struct ks8995_switch *ks8995;
+	int err;
 
 	dev = container_of(kobj, struct device, kobj);
 	ks8995 = dev_get_drvdata(dev);
 
-	return ks8995_read(ks8995, buf, off, count);
+	err = regmap_bulk_read(ks8995->regmap, off, buf, count);
+	return err ? : count;
 }
 
 static ssize_t ks8995_registers_write(struct file *filp, struct kobject *kobj,
@@ -306,11 +229,13 @@ static ssize_t ks8995_registers_write(struct file *filp, struct kobject *kobj,
 {
 	struct device *dev;
 	struct ks8995_switch *ks8995;
+	int err;
 
 	dev = container_of(kobj, struct device, kobj);
 	ks8995 = dev_get_drvdata(dev);
 
-	return ks8995_write(ks8995, buf, off, count);
+	err = regmap_bulk_write(ks8995->regmap, off, buf, count);
+	return err ? : count;
 }
 
 /* ks8995_get_revision - get chip revision
@@ -321,10 +246,10 @@ static ssize_t ks8995_registers_write(struct file *filp, struct kobject *kobj,
 static int ks8995_get_revision(struct ks8995_switch *ks)
 {
 	int err;
-	u8 id0, id1, ksz8864_id;
+	u32 id0, id1, ksz8864_id;
 
 	/* read family id */
-	err = ks8995_read_reg(ks, KS8995_REG_ID0, &id0);
+	err = regmap_read(ks->regmap, KS8995_REG_ID0, &id0);
 	if (err) {
 		err = -EIO;
 		goto err_out;
@@ -341,7 +266,7 @@ static int ks8995_get_revision(struct ks8995_switch *ks)
 	switch (ks->chip->family_id) {
 	case FAMILY_KS8995:
 		/* try reading chip id at CHIP ID1 */
-		err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1);
+		err = regmap_read(ks->regmap, KS8995_REG_ID1, &id1);
 		if (err) {
 			err = -EIO;
 			goto err_out;
@@ -354,7 +279,8 @@ static int ks8995_get_revision(struct ks8995_switch *ks)
 			ks->revision_id = get_chip_rev(id1);
 		} else if (get_chip_id(id1) != CHIPID_M) {
 			/* KSZ8864RMN */
-			err = ks8995_read_reg(ks, KS8995_REG_ID1, &ksz8864_id);
+			err = regmap_read(ks->regmap, KS8995_REG_ID1,
+					  &ksz8864_id);
 			if (err) {
 				err = -EIO;
 				goto err_out;
@@ -373,7 +299,7 @@ static int ks8995_get_revision(struct ks8995_switch *ks)
 		break;
 	case FAMILY_KSZ8795:
 		/* try reading chip id at CHIP ID1 */
-		err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1);
+		err = regmap_read(ks->regmap, KS8995_REG_ID1, &id1);
 		if (err) {
 			err = -EIO;
 			goto err_out;
@@ -429,6 +355,7 @@ static int ks8995_probe(struct spi_device *spi)
 {
 	struct ks8995_switch *ks;
 	int err;
+	struct regmap_config regmap_cfg;
 	int variant = spi_get_device_id(spi)->driver_data;
 
 	if (variant >= max_variant) {
@@ -487,6 +414,16 @@ static int ks8995_probe(struct spi_device *spi)
 		return err;
 	}
 
+	memcpy(&regmap_cfg, ks->chip->regmap_cfg, sizeof(regmap_cfg));
+	regmap_cfg.max_register = ks->chip->regs_size - 1;
+	ks->regmap = devm_regmap_init_spi(spi, &regmap_cfg);
+	if (IS_ERR(ks->regmap)) {
+		err = PTR_ERR(ks->regmap);
+		dev_err(&spi->dev, "Failed to allocate register map: %d\n",
+			err);
+		return err;
+	}
+
 	err = ks8995_get_revision(ks);
 	if (err)
 		return err;
-- 
1.9.1

Re: [PATCH v1 1/1] spi_ks8995: use regmap to access chip registers.

[David Miller]

From: Sven Van Asbroeck <svendev@arcx.com>
Date: Tue, 6 Feb 2018 10:13:56 -0500

> The register map layouts used in this driver are well suited to
> being accessed through a regmap. This makes the driver simpler
> and shorter, by eliminating some spi boilerplate code.
> 
> Testing:
> - tested on a ksz8785.
> - not tested on the other supported chips (ks8995, ksz8864)
>   because I don't have access to them.
>   However, I instrumented the spi layer to verify that the
>   correct spi transactions are generated to read the ID
>   registers on those chips.
> 
> Signed-off-by: Sven Van Asbroeck <svendev@arcx.com>

Please resubmit this simplification when the net-next tree opens
back up.

Thank you.

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Andrew Lunn]

On Tue, Feb 06, 2018 at 10:13:55AM -0500, Sven Van Asbroeck wrote:
> v1:
> 	starting point.
> 	is there a way to test-run this on supported devices that I don't
> 		have physical access to - (ks8995, ksz8864) ?
> 
> Sven Van Asbroeck (1):
>   spi_ks8995: use regmap to access chip registers.

Hi Sven

Rather than invest time in this driver, it would be better to look
into writing a DSA driver. There was some effort last year to make the
current microchip DSA driver more generic, but that has gone quiet
recently. Maybe you can talk to Tristram.Ha@microchip.com and
Woojung.Huh@microchip.com

	Andrew

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Sven Van Asbroeck]

On Tue, Feb 6, 2018 at 11:14 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> Rather than invest time in this driver, it would be better to look
> into writing a DSA driver.

Thank you Andrew. I know little of DSA, but at first sight it appears to
be a _very_ complicated beast for a switch PHY which only
needs a few static register settings applied to it on startup, and has
no further Linux interaction? That's how we use the part, anyway.
Not sure what its typical use case would be.

Woojung and Tristam, what do you think ?
ksz8795 datasheet:
http://ww1.microchip.com/downloads/en/DeviceDoc/00002112B.pdf

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Andrew Lunn]

On Tue, Feb 06, 2018 at 11:41:14AM -0500, Sven Van Asbroeck wrote:
> On Tue, Feb 6, 2018 at 11:14 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > Rather than invest time in this driver, it would be better to look
> > into writing a DSA driver.
> 
> Thank you Andrew. I know little of DSA, but at first sight it appears to
> be a _very_ complicated beast for a switch PHY which only
> needs a few static register settings applied to it on startup, and has
> no further Linux interaction?

Hi Sven

If you want to treat it as a dumb switch, then a simple driver is
sufficient. But it can do a lot more. Do you need spanning tree, or
are you happy for your network to collapse if there is a loop? Do you
want access to statistics? Know if links are up/down? VLAN support?
Save some power by enabling EEE? DSA will give you these features.

And a DSA driver does not need to be complex. You can start simple,
and add more features later.

     Andrew

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Sven Van Asbroeck]

On Tue, Feb 6, 2018 at 11:50 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> And a DSA driver does not need to be complex. You can start simple,
> and add more features later.

I see. Would it be possible/practical to start with just phy_read/write,
port_enable/disable in dsa_switch_ops ? And just add a sysfs bin file
for userspace register access ?

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Andrew Lunn]

On Tue, Feb 06, 2018 at 11:58:17AM -0500, Sven Van Asbroeck wrote:
> On Tue, Feb 6, 2018 at 11:50 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > And a DSA driver does not need to be complex. You can start simple,
> > and add more features later.
> 
> I see. Would it be possible/practical to start with just phy_read/write,
> port_enable/disable in dsa_switch_ops ? And just add a sysfs bin file
> for userspace register access ?

I would NACK sysfs bin file. Do it right, or don't do it at all.

I think i would first want to know what Tristam/Microchip plans
are. Does he intend to keep on working on the patches from last year?

       Andrew

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Florian Fainelli]

On 02/06/2018 09:05 AM, Andrew Lunn wrote:
> On Tue, Feb 06, 2018 at 11:58:17AM -0500, Sven Van Asbroeck wrote:
>> On Tue, Feb 6, 2018 at 11:50 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> And a DSA driver does not need to be complex. You can start simple,
>>> and add more features later.
>>
>> I see. Would it be possible/practical to start with just phy_read/write,
>> port_enable/disable in dsa_switch_ops ? And just add a sysfs bin file
>> for userspace register access ?
> 
> I would NACK sysfs bin file. Do it right, or don't do it at all.

Sven, there is a standard ethtool register dump interface that can be
used to provide register dumps, if necessary, that might fit your use case.

Aside from the phy_read/write, port_enable/disable, you will likely need
to have a get_tag_protocol() implementation returning
DSA_TAG_PROTO_NONE, and a setup() operation, other should be entirely
optional for now.

Out of curiosity, are there any break out boards with a KS8995 switch
available that we could e.g: plug to a Raspberry Pi or any similar board?

> 
> I think i would first want to know what Tristam/Microchip plans
> are. Does he intend to keep on working on the patches from last year?

Yes, this would be good to keep alive, the patches look good, they just
need to get in now.
-- 
Florian

Re: [PATCH v1 0/1] spi_ks8995: use regmap to access chip registers.

[Sven Van Asbroeck]

Andrew and Florian, thanks for your input.

On Tue, Feb 6, 2018 at 12:05 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> I would NACK sysfs bin file. Do it right, or don't do it at all.

On Tue, Feb 6, 2018 at 12:47 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Sven, there is a standard ethtool register dump interface that can be
> used to provide register dumps, if necessary, that might fit your use case.

I'm not keen on the sysfs bin file either, the existing driver has this,
and we use it because it's there.

Our use case is as follows: all we need is a reset on boot, followed by
a few static register bitfiddles which reflect the way we've integrated
the IC into our product. There is no further Linux interaction.
I guess the devicetree would be a natural place to store the required
register changes. No need for sysfs.

As I said before, not sure how others use this chip.

Would the above be attainable by a (trivial) DSA driver?