Re: nvmem: add driver for Microchip 24AA025E48 I2C eeprom / nodeID chip

[Felix Brack]

On 25.01.2018 15:37, Felix Brack wrote:
> 
> On 24.01.2018 17:02, Bartosz Golaszewski wrote:
>> 2018-01-24 16:18 GMT+01:00 Felix Brack <fb@xxxxxxx>:
>>>
>>> On 24.01.2018 16:08, Andy Shevchenko wrote:
>>>> On Wed, Jan 24, 2018 at 4:23 PM, Felix Brack <fb@xxxxxxx> wrote:
>>>>> Hello,
>>>>>
>>>>> About three years ago I wrote a driver for Microchip's 24AA025E48 I2C
>>>>> eeprom/nodeID chip. At that time I placed the source files in
>>>>> drivers/misc/eeprom. I never posted the code.
>>>>> I now plan to rewrite the driver from scratch. Is it correct to place
>>>>> the source code in drivers/nvmem and is there a special mailing list to
>>>>> which the patch should be posted when ready?
>>>>>
>>>>> many thanks and kind regards, Felix
>>>>
>>>> Does the existing driver [1] work for you (if you add ID there)?
>>>>
>>>> [1]: at24
>>>>
>>> Yes it does. Actually the driver I wrote 3 years ago is based on the
>>> at24 driver. Lot's of code in my driver originates directly from the
>>> at24 driver.
>>>
>>> --
>>> regards Felix
>>
>> Just from looking at the doc - it seems that it's a variant of
>> 24mac402. You should be able to access the memory block by
>> instantiating an 'atmel,24c02' device. As for the EUI-48 block -
>> current at24 driver will not work as the MAC is located at a different
>> offset in this chip. We need to figure out a portable way to specify
>> the addresses of such special blocks (same with the serial number) in
>> the at24 driver.
>>
>> Anyways - don't write your own driver for that, just make sure at24 works.
>>
> If no one offends against blowing up this driver by some more code, then
> that's fine with me. What about a patch that does the following:
> 
> 1. Extend the DT bindings by a new optional property block_offset
>    denoting where to start reading/writing from/to this device.
> 2. Add a member block_offset to struct at24_platform_data{} to store
>    the device read/write offset returned from the DT (similar to the
>    already existing page_size member).
> 3. Complement at24_get_pdata() to read block_offset from the DT and
>    store it.
> 4. Make at24_eeprom_write_i2c() and at24_eeprom_read_serial() (and
>    eventually more?) respect the block_offset value.
> 
> Initializing the new block_offset member of struct at24_platform_data{}
> to 0 should leave the code semantically unchanged.
> 
> Once this patch is in place I could use the 24mac402 device type with
> block_offset set to 0xfa (and read-only set to true) to read the EUI-48
> node address from Microchip's 24AA025E48. Probably the later addition of
> a device named 24eui48 would also make sense (if not redundant ?).
> 
> Any comments are very appreciated, Felix
> 
A major difference between Microchip's 24AA025E48 and Atmel's AT24MAC402
is the fact that the Atmel chip uses two _different_ I2C addresses: one
to access the eeprom space and one to access EUI/serial number space.
This allows to instantiate two drivers: a 24c02 (for eeprom access) and
an 24mac402 or 24mac602 (for EUI/serial number access).
Microchip's device however uses only one single I2C address to access
both, the eeprom (lower 1k bits) and the EUI (upper 1k bits) space.
Hence one driver has to manage both functions (eeprom/EUI) similar to a
mfd driver.

The driver I wrote myself 3 years ago basically does the same as the
at24 driver with respect to the eeprom functionality. In addition it
creates two files in the sysfs showing the EUI-48/64 as hex formatted
string.
One could argue that it is not the kernel's (or more precisely the
driver's) job to format data provide by some hardware device as this can
be done just as well or even better in user-space. For the device in
question I would fully agree. If, for example, one stores some fancy
looking data to the eeprom, does this justify a special driver which in
fact doesn't add more then format that data when read back? Definitely
NO! A serial number is just some fancy date, pre-programmed by the chip
manufacture, I admit.

I have therefore come to the conclusion that it is best to drop the idea
of an additional driver or the modification of the existing at24 driver.
I will just use the at24 driver to read the EUI-48 node ID as 6 bytes
and do all the rest in user-space.

many thanks for all your feedback, Felix