RE: [PATCH net-next 2/6] net: txgbe: Implement I2C bus master driver

["Jiawen Wu" <jiawenwu@trustnetic.com>]

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
>