Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Greg Patrick <gregspatrick@hotmail.com>]

Thanks for the review, Andrew -- folding your three replies together.

> > ... wires none of the cage sideband signals to a software-visible GPIO.
> Are they wired to a software invisible GPIO? Or nowhere at all?

Nowhere usable that I can find. TX_DISABLE is driven via a PCA9534 I/O
expander, but no presence/MOD_ABS line reaches a readable input: the
RTL9303 gpio0 lines the sister TL-ST1008F uses for MOD_DEF0 read stuck-low
here, the single PCA9534 is fully consumed by TX_DISABLE, and there is no
RTL8231. I haven't traced the PCB or decompiled the vendor firmware, so I
can't prove it goes literally nowhere versus somewhere I can't reach -- but
there is no software-readable presence source. I'll reword the commit
message to say that precisely.

> You might want to differentiate between an SFF and an SFP, using the
> compatible. An SFF has no need for an MOD_DEF0 GPIO. I did not look,
> maybe the code already does?

It doesn't currently, and you're right that my patch wrongly changes the
sff,sff case too: sff_data has no SFP_F_PRESENT in .gpios, so a soldered
sff,sff never has a MOD_DEF0 GPIO and always hits this branch. v2 keeps the
always-present path for sff,sff and uses the I2C probe only for an sff,sfp
cage with no MOD_DEF0 GPIO, gated on (sff->gpios & SFP_F_PRESENT).

> Did you try a 0 byte read? That is what tools like i2cdetect use.

I can't on this board: the cage I2C master is the RTL9303's SMBus
controller, which advertises only I2C_FUNC_SMBUS_BYTE_DATA -- no
I2C_FUNC_I2C and, notably, no SMBus Quick Command (the zero-length probe
i2cdetect uses). sfp_i2c_configure() therefore selects sfp_smbus_byte_read()
with i2c_max_block_size = 1, and the presence probe reuses that same path.
A 1-byte read is the lightest primitive this controller supports, and it is
also the only form that works through sfp->read on both raw-I2C and
SMBus-only adapters -- a zero-length transfer is not expressible there. (I
checked on the hardware: i2ctransfer reports "Adapter does not have I2C
transfers capability", and i2cdetect -F shows Quick Command unsupported but
Receive/Read Byte supported, which is what presence detection rides on.)

> > -static unsigned int sff_gpio_get_state(struct sfp *sfp)
> Humm. Why?

That deletion was wrong, for the sff,sff reason above -- I'll keep
sff_gpio_get_state() in v2.

I'll send the v2 with these changes after the 24h window. Thanks again.

Greg