Am 26.11.2017 um 21:27 schrieb Bartosz Golaszewski:
> 2017-11-24 23:13 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>:
>> 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@xxxxxxxxx>:
>>>>>> 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.
>
I think it's opposite, 24MAC402 causes the trouble due to byte_len = 6.
Approach is fine with me, are you going to prepare the fix for
replacing the magic for 24MAC402 ?
> 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?
>
Also fine with me, then I will send the (hopefully) final series
tonight incl. the last small requested change.
> 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/
>>>>>
>>>>
>>>
>>
>
