[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 4 Apr 2023 10:47:28 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Andrew Lunn'" <andrew@...n.ch>
Cc: <netdev@...r.kernel.org>, <linux@...linux.org.uk>,
<mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next 2/6] net: txgbe: Implement I2C bus master driver
On Monday, April 3, 2023 8:53 PM, Andrew Lunn wrote:
> On Mon, Apr 03, 2023 at 02:45:24PM +0800, Jiawen Wu wrote:
> > I2C bus is integrated in Wangxun 10Gb ethernet chip. Implement I2C bus
> > driver to receive I2C messages.
>
> Please Cc: the i2c mailing list for comments. They know more about I2C than
the
> netdev people.
>
> Is the I2C bus master your own IP, or have you licensed a core? Or using the
open
> cores i2C bus master? I just want to make sure there is not already a linux
driver for
> this.
>
I use the I2C core driver, and implement my own i2c_algorithm. I think it needs
to configure my registers to realize the function.
> > +static void txgbe_i2c_start(struct wx *wx, u16 dev_addr) {
> > + wr32(wx, TXGBE_I2C_ENABLE, 0);
> > +
> > + wr32(wx, TXGBE_I2C_CON,
> > + (TXGBE_I2C_CON_MASTER_MODE |
> > + TXGBE_I2C_CON_SPEED(1) |
> > + TXGBE_I2C_CON_RESTART_EN |
> > + TXGBE_I2C_CON_SLAVE_DISABLE));
> > + /* Default addr is 0xA0 ,bit 0 is configure for read/write! */
>
> A generic I2C bus master should not care about that address is being
read/write.
> For the SFP use case, 0xa0 will be used most of the time, plus 0xa2 for
diagnostics.
> But when the SFP contains a copper PHY, other addresses will be used as well.
>
Yes, this comment is redundant. The address will be set to 'msg[0].addr'.
> > +static int txgbe_i2c_xfer(struct i2c_adapter *i2c_adap,
> > + struct i2c_msg *msg, int num_msgs) {
> > + struct wx *wx = i2c_get_adapdata(i2c_adap);
> > + u8 *dev_addr = msg[0].buf;
> > + bool read = false;
> > + int i, ret;
> > + u8 *buf;
> > + u16 len;
> > +
> > + txgbe_i2c_start(wx, msg[0].addr);
> > +
> > + for (i = 0; i < num_msgs; i++) {
> > + if (msg[i].flags & I2C_M_RD) {
> > + read = true;
> > + len = msg[i].len;
> > + buf = msg[i].buf;
> > + }
> > + }
> > +
> > + if (!read) {
> > + wx_err(wx, "I2C write not supported\n");
> > + return num_msgs;
> > + }
>
> Write is not supported at all? Is this a hardware limitation? I think
-EOPNOTSUPP
> is required here, and you need to ensure the code using the I2C bus master has
> quirks to not try to write.
It is supported. False testing leads to false perceptions, I'll fix it.
>
> > +#define TXGBE_I2C_SLAVE_ADDR (0xA0 >> 1)
> > +#define TXGBE_I2C_EEPROM_DEV_ADDR 0xA0
>
> These two do not appear to be used? I guess you took your hard coded SFP i2c
bus
> master and generalised it? Please clean up dead code like this.
>
> Andrew
>
Powered by blists - more mailing lists