| Message ID | 20210202191645.439-1-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: xrs700x: Correctly address device over I2C | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 64 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 2, 2021 at 1:17 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: [snip] > > George, have you used the chip in I2C mode with the code that is on > net-next now? I was not able to get the driver to even read the ID > register correctly. I wrote the i2c driver before I had any hardware in hand thinking I was going to get a board with the switch connected via i2c. When the board arrived it turned out it was connected via mdio so I wrote that driver as well. I looked it over quite carefully but I guess the documentation was wrong and I had the register addresses shifted off by one. I never ended up with hardware to test the i2c. > > drivers/net/dsa/xrs700x/xrs700x_i2c.c | 31 ++++++++++++--------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/dsa/xrs700x/xrs700x_i2c.c b/drivers/net/dsa/xrs700x/xrs700x_i2c.c > index a5f8883af829..16a46a78a037 100644 > --- a/drivers/net/dsa/xrs700x/xrs700x_i2c.c > +++ b/drivers/net/dsa/xrs700x/xrs700x_i2c.c > @@ -10,33 +10,34 @@ > #include "xrs700x.h" > #include "xrs700x_reg.h" > > +struct xrs700x_i2c_cmd { > + __be32 reg; > + __be16 val; > +} __packed; > + > static int xrs700x_i2c_reg_read(void *context, unsigned int reg, > unsigned int *val) > { > struct device *dev = context; > struct i2c_client *i2c = to_i2c_client(dev); > - unsigned char buf[4]; > + struct xrs700x_i2c_cmd cmd; > int ret; > > - buf[0] = reg >> 23 & 0xff; > - buf[1] = reg >> 15 & 0xff; > - buf[2] = reg >> 7 & 0xff; > - buf[3] = (reg & 0x7f) << 1; > + cmd.reg = cpu_to_be32(reg | 1); > > - ret = i2c_master_send(i2c, buf, sizeof(buf)); > + ret = i2c_master_send(i2c, (char *)&cmd.reg, sizeof(cmd.reg)); > if (ret < 0) { > dev_err(dev, "xrs i2c_master_send returned %d\n", ret); > return ret; > } > > - ret = i2c_master_recv(i2c, buf, 2); > + ret = i2c_master_recv(i2c, (char *)&cmd.val, sizeof(cmd.val)); > if (ret < 0) { > dev_err(dev, "xrs i2c_master_recv returned %d\n", ret); > return ret; > } > > - *val = buf[0] << 8 | buf[1]; > - > + *val = be16_to_cpu(cmd.val); > return 0; > } > > @@ -45,17 +46,13 @@ static int xrs700x_i2c_reg_write(void *context, unsigned int reg, > { > struct device *dev = context; > struct i2c_client *i2c = to_i2c_client(dev); > - unsigned char buf[6]; > + struct xrs700x_i2c_cmd cmd; > int ret; > > - buf[0] = reg >> 23 & 0xff; > - buf[1] = reg >> 15 & 0xff; > - buf[2] = reg >> 7 & 0xff; > - buf[3] = (reg & 0x7f) << 1 | 1; > - buf[4] = val >> 8 & 0xff; > - buf[5] = val & 0xff; > + cmd.reg = cpu_to_be32(reg); > + cmd.val = cpu_to_be16(val); > > - ret = i2c_master_send(i2c, buf, sizeof(buf)); > + ret = i2c_master_send(i2c, (char *)&cmd, sizeof(cmd)); > if (ret < 0) { > dev_err(dev, "xrs i2c_master_send returned %d\n", ret); > return ret; > -- > 2.17.1 > Reviewed-by: George McCollister <george.mccollister@gmail.com>
On Tue, 2 Feb 2021 14:05:20 -0600 George McCollister wrote: > On Tue, Feb 2, 2021 at 1:17 PM Tobias Waldekranz <tobias@waldekranz.com> wrote: > [snip] > > > > George, have you used the chip in I2C mode with the code that is on > > net-next now? I was not able to get the driver to even read the ID > > register correctly. > > I wrote the i2c driver before I had any hardware in hand thinking I > was going to get a board with the switch connected via i2c. When the > board arrived it turned out it was connected via mdio so I wrote that > driver as well. I looked it over quite carefully but I guess the > documentation was wrong and I had the register addresses shifted off > by one. I never ended up with hardware to test the i2c. > Reviewed-by: George McCollister <george.mccollister@gmail.com> Applied, thanks!
diff --git a/drivers/net/dsa/xrs700x/xrs700x_i2c.c b/drivers/net/dsa/xrs700x/xrs700x_i2c.c index a5f8883af829..16a46a78a037 100644 --- a/drivers/net/dsa/xrs700x/xrs700x_i2c.c +++ b/drivers/net/dsa/xrs700x/xrs700x_i2c.c @@ -10,33 +10,34 @@ #include "xrs700x.h" #include "xrs700x_reg.h" +struct xrs700x_i2c_cmd { + __be32 reg; + __be16 val; +} __packed; + static int xrs700x_i2c_reg_read(void *context, unsigned int reg, unsigned int *val) { struct device *dev = context; struct i2c_client *i2c = to_i2c_client(dev); - unsigned char buf[4]; + struct xrs700x_i2c_cmd cmd; int ret; - buf[0] = reg >> 23 & 0xff; - buf[1] = reg >> 15 & 0xff; - buf[2] = reg >> 7 & 0xff; - buf[3] = (reg & 0x7f) << 1; + cmd.reg = cpu_to_be32(reg | 1); - ret = i2c_master_send(i2c, buf, sizeof(buf)); + ret = i2c_master_send(i2c, (char *)&cmd.reg, sizeof(cmd.reg)); if (ret < 0) { dev_err(dev, "xrs i2c_master_send returned %d\n", ret); return ret; } - ret = i2c_master_recv(i2c, buf, 2); + ret = i2c_master_recv(i2c, (char *)&cmd.val, sizeof(cmd.val)); if (ret < 0) { dev_err(dev, "xrs i2c_master_recv returned %d\n", ret); return ret; } - *val = buf[0] << 8 | buf[1]; - + *val = be16_to_cpu(cmd.val); return 0; } @@ -45,17 +46,13 @@ static int xrs700x_i2c_reg_write(void *context, unsigned int reg, { struct device *dev = context; struct i2c_client *i2c = to_i2c_client(dev); - unsigned char buf[6]; + struct xrs700x_i2c_cmd cmd; int ret; - buf[0] = reg >> 23 & 0xff; - buf[1] = reg >> 15 & 0xff; - buf[2] = reg >> 7 & 0xff; - buf[3] = (reg & 0x7f) << 1 | 1; - buf[4] = val >> 8 & 0xff; - buf[5] = val & 0xff; + cmd.reg = cpu_to_be32(reg); + cmd.val = cpu_to_be16(val); - ret = i2c_master_send(i2c, buf, sizeof(buf)); + ret = i2c_master_send(i2c, (char *)&cmd, sizeof(cmd)); if (ret < 0) { dev_err(dev, "xrs i2c_master_send returned %d\n", ret); return ret;
On read, master should send 31 MSB of the register (only even values are ever used), followed by a 1 to indicate read. Then, reading two bytes, the device will output the register's value. On write, master sends 31 MSB of the register, followed by a 0 to indicate write, followed by two bytes containing the register value. Flexibilis' documentation (version 1.3) specifies the opposite polarity (#read/write), but the scope indicates that it is, in fact, read/#write. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- George, have you used the chip in I2C mode with the code that is on net-next now? I was not able to get the driver to even read the ID register correctly. drivers/net/dsa/xrs700x/xrs700x_i2c.c | 31 ++++++++++++--------------- 1 file changed, 14 insertions(+), 17 deletions(-)