Re: [PATCH net-next v5] net: sfp: extend SMBus support

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Jonas,

On 16/01/2026 12:31, Jonas Jelonek wrote:
> Commit 7662abf4db94 ("net: phy: sfp: Add support for SMBus module access")
> added support for SMBus-only controllers for module access. However, this
> is restricted to single-byte accesses and has the implication that hwmon
> is disabled (due to missing atomicity of 16-bit accesses) and warnings
> are printed.
> 
> There are probably a lot of SMBus-only I2C controllers out in the wild
> which support more than just byte access. And it also seems that in
> several devices, SFP slots are attached to these SMBus controllers
> instead of full-featured I2C controllers. Right now, they don't work
> with SFP modules. This applies - amongst others - to I2C/SMBus-only
> controllers in Realtek longan and mango SoCs. They also support word
> access and I2C block reads.
> 
> Extend the current read/write SMBus operations to support SMBus I2C
> block and SMBus word access. To avoid having dedicated operations for
> each kind of transfer, provide generic read and write operations that
> covers all kinds of access depending on whats supported.
> 
> For block access, this requires I2C_FUNC_SMBUS_I2C_BLOCK to be
> supported as it relies on reading a pre-defined amount of bytes.
> This isn't intended by the official SMBus Block Read but supported by
> several I2C controllers/drivers.

First of all, thanks for this new version :)

[...]

> +static int sfp_smbus_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> +			   size_t len)
>  {
>  	union i2c_smbus_data smbus_data;
>  	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t this_len, transferred;
> +	u32 functionality;
>  	u8 *data = buf;
>  	int ret;
>  
> +	functionality = i2c_get_functionality(sfp->i2c);
> +
>  	while (len) {
> -		smbus_data.byte = *data;
> -		ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> -				     I2C_SMBUS_WRITE, dev_addr,
> -				     I2C_SMBUS_BYTE_DATA, &smbus_data);
> -		if (ret)
> +		this_len = min(len, sfp->i2c_max_block_size);
> +
> +		if (this_len > 2 &&
> +		    functionality & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK) {
> +			smbus_data.block[0] = this_len;
> +			memcpy(&smbus_data.block[1], data, this_len);
> +
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_WRITE, dev_addr,
> +					     I2C_SMBUS_WORD_DATA, &smbus_data);
> +			transferred = this_len;
> +		} else if (this_len >= 2 &&
> +			   functionality & I2C_FUNC_SMBUS_WRITE_WORD_DATA) {
> +			smbus_data.word = get_unaligned_le16(data);
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_WRITE, dev_addr,
> +					     I2C_SMBUS_WORD_DATA, &smbus_data);
> +			transferred = 2;
> +		} else {
> +			smbus_data.byte = *data;
> +			ret = i2c_smbus_xfer(sfp->i2c, bus_addr, 0,
> +					     I2C_SMBUS_WRITE, dev_addr,
> +					     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +			transferred = 1;
> +		}
> +
> +		if (ret < 0)
>  			return ret;
>  
> -		len--;
> -		data++;
> -		dev_addr++;
> +		data += transferred;
> +		len -= transferred;
> +		dev_addr += transferred;
>  	}

I think Russell pointed it out, but I was also wondering the same.
How do we deal with controllers that cannot do neither block nor
single-byte, i.e. that can only do word access ?

We can't do transfers that have an odd length. And there are some,
see sfp_cotsworks_fixup_check() for example.

Maybe these smbus controller don't even exist, but I think we should
anyway have some log saying that this doesn't work, either at SFP
access time, or at init time.

Maxime