Hi Vidya, Thanks a lot for your your work! Here are my 2 cents:
> + /* SFP voltage in 0.1mV units */ > + __u16 sfp_voltage; > + /* SFP Temp in 16-bit signed 1/256 Celcius */ > + __s16 sfp_temp; > + /* [4] tables are low/high warn, low/high alarm */ You already had just 5 lines earlier: /* SFP voltage in 0.1mV units */ Shouldn't it be: /* SFP threshold voltage in 0.1mV units */ ? > + /* SFP voltage in 0.1mV units */ > + __u16 thresh_sfp_voltage[4]; pagging should be: paging, support_alarms should be:supports_alarms > + * If pagging support exists, then support_alarms is marked as 1 > + */ > + + if (eeprom_len == ETH_MODULE_SFF_8636_MAX_LEN) { + if (!(id[SFF8636_STATUS_2_OFFSET] & + SFF8636_STATUS_PAGE_3_PRESENT)) { + sd.supports_alarms = 1; + } + } Shouldn't it be power/ TX bias current fields? ("current" only once) > + /* > + * SFF-8636/8436 spec is not clear whether RX power/ TX bias current > + * current fields are supported or not. A valid temperature reading > + * is used as existence for TX/RX power. Should it be: SFF-8472/8079 (without "$")? > + * > + * Common utilities across SFF-8436/8636 and SFF-8472/8079$ > + * are defined in this file > + * Regards, Rami Rosen