> > +#define MODULE_EEPROM_PAGE 0 > > +#define MODULE_EEPROM_OFFSET 0 > > +#define MODULE_EEPROM_LENGTH 1 > > +#define MODULE_EEPROM_I2C_ADDR 0x50 > > + > > +static int module_flash_fw_work_init(struct ethtool_module_fw_flash > *module_fw, > > + struct net_device *dev, > > + struct netlink_ext_ack *extack) { > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > + struct ethtool_module_eeprom page_data = {}; > > + struct module_sff8024_id_rpl *rpl; > > + int err; > > + > > + /* Fetch the SFF-8024 Identifier Value. For all supported standards, it > > + * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024, > > + * revision 4.9. > > + */ > > + page_data.page = MODULE_EEPROM_PAGE; > > + page_data.offset = MODULE_EEPROM_OFFSET; > > + page_data.length = MODULE_EEPROM_LENGTH; > > + page_data.i2c_address = MODULE_EEPROM_I2C_ADDR; > > Please use better names - these aren't any better than using integers. > > Maybe use SFP_PHYS_ID for the offset? > > > + page_data.data = kmalloc(page_data.length, GFP_KERNEL); > > + if (!page_data.data) > > + return -ENOMEM; > > + > > + err = ops->get_module_eeprom_by_page(dev, &page_data, extack); > > + if (err < 0) > > + goto out; > > + > > + rpl = (struct module_sff8024_id_rpl *)page_data.data; > > What purpose does this structure of a single byte serve? To me, it just > obfuscates the code. > > u8 phys_id; > > ... > page_data.offset = SFP_PHYS_ID; > page_data.length = sizeof(phys_id); > page_data.data = &phys_id; > ... > switch (phys_id) { > > will work just as well, and be more explicit about what's actually going on > here. It doesn't mean that I have to understand what this new > module_sff8024_id_rpl structure is. I can see that we're just getting one byte > which is the module physical ID. > > You also then don't need to care about kfree()ing one byte of data structure. OK, will change, thanks! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!