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

Reply via email to