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:
>>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@xxxxxxxxx>:
>>>>> Add regmap-based read function and instead of using three different
>>>>> read functions (standard, mac, serial) use just one and factor out the
>>>>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>>> ---
>>>>> v2:
>>>>> - rebased
>>>>> v3:
>>>>> - improve readability
>>>>> - re-introduce debug message
>>>>> - introduce at24_adjust_read_offset
>>>>> v4:
>>>>> - move offset adjustment calculation to probe function
>>>>> ---
>>>>> drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 55 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>>> index 493e2b646..c16a9a495 100644
>>>>> --- a/drivers/misc/eeprom/at24.c
>>>>> +++ b/drivers/misc/eeprom/at24.c
>>>>> @@ -75,6 +75,7 @@ struct at24_data {
>>>>>
>>>>> unsigned write_max;
>>>>> unsigned num_addresses;
>>>>> + unsigned int offset_adj;
>>>>>
>>>>> struct nvmem_config nvmem_config;
>>>>> struct nvmem_device *nvmem;
>>>>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>>>> return -ETIMEDOUT;
>>>>> }
>>>>>
>>>>
>>>> OK this looks better. The series is almost ready - just a couple more
>>>> nits I'd like to see fixed and we're done.
>>>>
>>>>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>>>>> + unsigned int offset, size_t count)
>>>>> +{
>>>>> + unsigned long timeout, read_time;
>>>>> + struct at24_client *at24_client;
>>>>> + struct i2c_client *client;
>>>>> + struct regmap *regmap;
>>>>> + int ret;
>>>>> +
>>>>> + at24_client = at24_translate_offset(at24, &offset);
>>>>> + regmap = at24_client->regmap;
>>>>> + client = at24_client->client;
>>>>> +
>>>>> + if (count > io_limit)
>>>>> + count = io_limit;
>>>>> +
>>>>> + /* adjust offset for mac and serial read ops */
>>>>> + offset += at24->offset_adj;
>>>>
>>>> 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.
>>> 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/
>>
>
