Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Bartosz Golaszewski <brgl@bgdev.pl>]

2017-11-24 23:13 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 24.11.2017 um 22:17 schrieb Heiner Kallweit:
>> Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
>>>
>>>
>>> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>>>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>>>> in both sides).
>>>>>>
>>>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>>>> some datasheets, what lead to quite some question marks ..
>>>>>
>>>>> Major issue is that offset and size in at24_read/write are not checked
>>>>> currently. So we completely rely on the calling subsystem (nvmem).
>>>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>>>> does not. So maybe the nvmem core should be changed to do checking in
>>>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>>>> to hear his opinion.
>>>>>
>>>>> If we have such checks then in general |= and += deliver the same result,
>>>>> it's just a question of taste.
>>>>>
>>>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>>>
>>>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
>>> Moreover, if I remember good, in the initialization of the 24mac402 the
>>> size is truncated at something which is power of 2. I don't if this is
>>> for some historical reasons or not so that you can only read 4 bytes
>>> instead of 6 for the EUI-48.
>>>
>> Very good point! Actually I don't see any real need for this check.
>> I thin we lose nothing if we simply remove it.
>>
> Just see that this check only prints a warning, the actual issue comes
> from the ilog2 in AT24_DEVICE_MAGIC.
> When we need one more config parameter anyway (for the MAC start address),
> then IMO it would make sense to convert the magic to a proper struct.
> I'll spend a few thoughts on that.
>

One thing that bothers me is that we now have a feature in the kernel
(reading the MAC address from at24mac402/at24mac604) which doesn't
work and any patch fixing it, that at the same time significantly
changes the code would likely not make its way into the stable
branches.

I see it like this: I would like to merge commit ("eeprom: at24: fix
reading from 24MAC402/24MAC602") first: that would at least fix the
at24mac402 case. For at24mac602 it would be ok to create the MAGIC
manually without the call to ilog2(). Such changes would then be
submitted for linux-stable.

After these two patches, I will merge the regmap conversion patches
and we would base any subsequent development (e.g. magic -> struct
conversion) on top of that. How about that?

Best regards,
Bartosz Golaszewski

>>>>> So the caller has to provide offset 0x08 to read the mac what is
>>>>> greater than the chip size of 6 bytes.
>>>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>>>
>>>>> I saw that you submitted the 24macx02 code, did you test the driver
>>>>> with one of these chips and I miss something?
>>>>>
>>>>
>>>> At the time when I submitted the support for at24cs (which I had
>>>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>>>> include support for at24mac too, but since I don't have such a chip, I
>>>> could not really test it. Looking at the note on page 21 of the
>>>> relevant datasheet, it's obvious it can't work. I must have missed
>>>> that at the time of writing the code.
>>>>
>>>> Also: there's this patch[1] which looks like a workaround for this
>>>> problem. I'm Cc'ing the author.
>>> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
>>> and providing this offset via device tree was my first option in order
>>> to not broke the initial functionality. Anyway, the device tree approach
>>> as not accepted at that time, the usage of another DT binding was proposed
>>> to me at that time but I didn't found that feasible, said about it on
>>> mailing list but I didn't received any other inputs.
>>>
>>
>> My patch works for the two MAC EEPROM's currently supported by the driver,
>> but not for others like the one mentioned by you (Microchip 24AA02E48 and
>> friends) because they have other start addresses for the MAC.
>>
>> To deal with this situation we would have to add the MAC start address to
>> the chip config data. In addition the proposed DT parameter would helpful
>> in case chips are used which are not yet supported by the driver
>> (similar to the recently introduced "size" parameter).
>>
>>
>> Bartosz, can we first go with the additional sanity checking in
>> at24_read/write (if fine with you) and my i2c refactoring
>> (will resubmit with the last small change)?
>> Then the driver is somewhat smaller and simpler what makes further
>> improvements easier.
>>
>> Kind regards,
>> Heiner
>>
>>>>
>>>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>>>> the MAC at a different offset? Could you by any chance test the
>>>> patch[2] from Heiner?
>>> I have chip at [3] with MAC at 0xfa.
>>>
>>> Regarding the testing of patch [2], at this moment I haven't a board
>>> with at24mac602 EEPROM. I will come back later to this thread as soon as
>>> I will have one. Regarding the changes, if I remember good, the
>>> at24->chip.byte_len is truncated at something which is power of 2,
>>> in case of 24mac402 will be 4 not 6 as expected, so it should return
>>> only 4 LSB bytes of MAC. Other than this it looks OK from my point
>>> of view.
>>>
>>> Thanks,
>>> Claudiu
>>>
>>> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>>>
>>>>> Most likely we would have to change the driver so that the caller can
>>>>> read the mac from offset 0.
>>>>>
>>>>> Rgds, Heiner
>>>>>
>>>>
>>>> Best regards,
>>>> Bartosz Golaszewski
>>>>
>>>> [1] http://patchwork.ozlabs.org/patch/785106/
>>>> [2] http://patchwork.ozlabs.org/patch/840958/
>>>>
>>>
>>
>