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

[David Miller]

From: Sven Van Asbroeck 
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 

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

Thank you.

[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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -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 = _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 = _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 = _regmap_cfg,
},
 };
 
@@ -152,6 +166,7 @@ struct ks8995_switch {
struct bin_attributeregs_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();
-
-   memset(, 0, sizeof(t));
-
-   t[0].tx_buf = 
-   t[0].len = sizeof(cmd);
-   spi_message_add_tail([0], );
-
-   t[1].rx_buf = buf;
-   t[1].len = count;
-   spi_message_add_tail([1], );
-
-   mutex_lock(>lock);
-   err = spi_sync(ks->spi, );
-   mutex_unlock(>lock);
-
-   return err ? err : count;
-}
-
-static int ks8995_write(struct ks8995_switch *ks, char *buf,
-unsigned offset, size_t count)
-{
-