2016-04-16 23:17 GMT+02:00 Wolfram Sang <wsa@xxxxxxxxxxxxx>: > Hi, > > On Mon, Apr 11, 2016 at 11:57:16AM -0700, Bartosz Golaszewski wrote: >> Chips from the at24cs EEPROM series have an additional read-only >> memory area containing a factory pre-programmed serial number. In >> order to access it, a dummy write must be executed before reading >> the serial number bytes. >> >> Chips from the at24mac familiy, apart from the serial number, have >> a second special memory area containing a factory programmed >> EUI-48/EUI64 mac address. >> >> The read-only serial/mac memory area is accessible on a different i2c >> slave address (addr + 0x08). This patchset makes it possible to >> instantiate a separate at24 device on this second address and access >> the read-only area through the regular eeprom sysfs attribute or the >> new nvmem subsystem. >> >> This series also contains several patches intoducing some minor tweaks >> and refactoring. >> >> Tested with at24cs32 and at24cs02 chips (for both 16 and 8 bit address >> pointers). I have no means of testing the support for at24mac chips, I >> relied solely on the datasheet. > > Thanks for adding at24mac nonetheless! > > I still don't understand why you want to add this to at24 instead of > doing a seperate driver for it? You basically tie a read_function to an > nvmem device. You don't need the special smbus handling, cutting large > requests into chunks, write support, size flags and all this from at24. > And with the function pointers, it makes the already complex code a bit > more complex. > Hi Wolfram, while you're right that this makes the code more complex, I can see some space for overall improvement of the driver's code quality with function pointers. The read and write routines are quite complex and decide which operations to use (smbus or i2c) on the fly every time they're called. Why not split them into several simpler functions (each implementing a single way of talking to EEPROM) that would be assigned in probe() depending on the chip type and adapter's functionalities? Would you be willing to accept a series that does it? > IMO, this could be solved by a simple, seperate driver. You could start > with at24 and remove unneeded stuff. I think there should be mainly > nvmem-init code and your read routines left. I also think this new > driver should be allowed to set the "compat" option of the nvmem > framework, so you'll get the "eeprom" file, too. > > Or am I overlooking something? > I think creating two new drivers for something as simple as that is overkill. Especially when it's a backwards compatible sub-family of chips we're talking about. How about leaving this in at24 and instead cleaning the driver up a bit with the approach I suggested above? Best regards, Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
