| Message ID | 011081f7-f2af-0e78-f21b-c40493d63a70@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bartosz Golaszewski |
| Headers | show |
| Series | eeprom: at24: switch driver to regmap_i2c | expand |
2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: > 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@gmail.com> > --- > 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). > + > + loop_until_timeout(timeout, read_time) { > + ret = regmap_bulk_read(regmap, offset, buf, count); > + dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", > + count, offset, ret, jiffies); > + if (!ret) > + return count; > + } > + > + return -ETIMEDOUT; > +} > + > static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf, > unsigned int offset, size_t count) > { > @@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) > while (count) { > int status; > > - status = at24->read_func(at24, buf, off, count); > + status = at24_regmap_read(at24, buf, off, count); > if (status < 0) { > mutex_unlock(&at24->lock); > pm_runtime_put(&client->dev); > @@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip) > } > } > > +static unsigned int at24_get_offset_adj(u8 flags) > +{ > + if (flags & AT24_FLAG_MAC) > + return 0x90; Let's stay consistent here and do BIT(4) | BIT(7) > + else if (flags & AT24_FLAG_SERIAL && flags & AT24_FLAG_ADDR16) > + /* > + * For 16 bit address pointers, the word address must contain > + * a '10' sequence in bits 11 and 10 regardless of the > + * intended position of the address pointer. > + */ > + return BIT(11); Even though we only have a single line of code here and below, the multi-line comment makes it seem like a block of code. Please wrap it in braces so that nobody ever falls into a trap should someone want to modify in the future. > + else if (flags & AT24_FLAG_SERIAL) > + /* > + * Otherwise the word address must begin with a '10' sequence, > + * regardless of the intended address. > + */ > + return BIT(7); > + else > + return 0; > +} > + > static const struct regmap_config regmap_config_8 = { > .reg_bits = 8, > .val_bits = 8, > @@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > return -EINVAL; > } > > + at24->offset_adj = at24_get_offset_adj(chip.flags); > + > if (chip.flags & AT24_FLAG_SERIAL) { > at24->read_func = at24_eeprom_read_serial; > } else if (chip.flags & AT24_FLAG_MAC) { > -- > 2.15.0 > >
Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski: > 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: >> 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@gmail.com> >> --- >> 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. 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? Most likely we would have to change the driver so that the caller can read the mac from offset 0. Rgds, Heiner >> + >> + loop_until_timeout(timeout, read_time) { >> + ret = regmap_bulk_read(regmap, offset, buf, count); >> + dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", >> + count, offset, ret, jiffies); >> + if (!ret) >> + return count; >> + } >> + >> + return -ETIMEDOUT; >> +} >> + >> static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf, >> unsigned int offset, size_t count) >> { >> @@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) >> while (count) { >> int status; >> >> - status = at24->read_func(at24, buf, off, count); >> + status = at24_regmap_read(at24, buf, off, count); >> if (status < 0) { >> mutex_unlock(&at24->lock); >> pm_runtime_put(&client->dev); >> @@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip) >> } >> } >> >> +static unsigned int at24_get_offset_adj(u8 flags) >> +{ >> + if (flags & AT24_FLAG_MAC) >> + return 0x90; > > Let's stay consistent here and do BIT(4) | BIT(7) > >> + else if (flags & AT24_FLAG_SERIAL && flags & AT24_FLAG_ADDR16) >> + /* >> + * For 16 bit address pointers, the word address must contain >> + * a '10' sequence in bits 11 and 10 regardless of the >> + * intended position of the address pointer. >> + */ >> + return BIT(11); > > Even though we only have a single line of code here and below, the > multi-line comment makes it seem like a block of code. Please wrap it > in braces so that nobody ever falls into a trap should someone want to > modify in the future. > >> + else if (flags & AT24_FLAG_SERIAL) >> + /* >> + * Otherwise the word address must begin with a '10' sequence, >> + * regardless of the intended address. >> + */ >> + return BIT(7); >> + else >> + return 0; >> +} >> + >> static const struct regmap_config regmap_config_8 = { >> .reg_bits = 8, >> .val_bits = 8, >> @@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) >> return -EINVAL; >> } >> >> + at24->offset_adj = at24_get_offset_adj(chip.flags); >> + >> if (chip.flags & AT24_FLAG_SERIAL) { >> at24->read_func = at24_eeprom_read_serial; >> } else if (chip.flags & AT24_FLAG_MAC) { >> -- >> 2.15.0 >> >> >
2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: > Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski: >> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: >>> 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@gmail.com> >>> --- >>> 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. > 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. @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? > 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/
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: >>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: >>>> 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@gmail.com> >>>> --- >>>> 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. >> 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. > > @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/ >
On 23/11/17 21:31, Heiner Kallweit wrote: > 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.w.r.t nvmem I can see these sanity check are missing form read/writes. we should probably add the check when we add the cell itself something like this below should do the job.. I will send a proper patch after testing.. ------------------------>cut<-------------------------- diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index d12e5de78e70..8ae865765754 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -320,11 +320,26 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem) } } -static void nvmem_cell_add(struct nvmem_cell *cell) +static int nvmem_cell_add(struct nvmem_cell *cell) { + struct nvmem_device *nvmem = cell->nvmem; + + if (cell->offset >= nvmem->size) + return -EINVAL; + + if (cell->bytes < nvmem->word_size) + return -EINVAL; + + if (cell->offset + cell->bytes > nvmem->size) + cell->bytes = nvmem->size - cell->offset; + + cell->bytes = round_down(cell->bytes, nvmem->word_size); + mutex_lock(&nvmem_cells_mutex); list_add_tail(&cell->node, &nvmem_cells); mutex_unlock(&nvmem_cells_mutex); + + return 0; } static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem, @@ -377,7 +392,11 @@ static int nvmem_add_cells(struct nvmem_device *nvmem, goto err; } - nvmem_cell_add(cells[i]); + rval = nvmem_cell_add(cells[i]); + if (rval) { + kfree(cells[i]); + goto err; + } } nvmem->ncells = cfg->ncells; @@ -830,7 +849,9 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, goto err_sanity; } - nvmem_cell_add(cell); + rval = nvmem_cell_add(cell); + if (rval) + goto err_sanity; return cell; ------------------------>cut<--------------------------
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: >>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: >>>>> 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@gmail.com> >>>>> --- >>>>> 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/ >> >
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: >>>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: >>>>>> 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@gmail.com> >>>>>> --- >>>>>> 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. > 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. >>>> 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/ >>> >> >
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/ >>>> >>> >> >
Am 26.11.2017 um 21:27 schrieb Bartosz Golaszewski: > 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. > 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/ >>>>> >>>> >>> >> >
2017-11-27 7:24 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>: > Am 26.11.2017 um 21:27 schrieb Bartosz Golaszewski: >> 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. >> > I think it's opposite, 24MAC402 causes the trouble due to byte_len = 6. Indeed. > Approach is fine with me, are you going to prepare the fix for > replacing the magic for 24MAC402 ? > Yes, I'll post it later today. >> 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. > Ok, I'll give it a last testing spin on top of 4.15-rc1 and hopefully we can have it in by Wednesday. Thanks, Bartosz >> 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/ >>>>>> >>>>> >>>> >>> >> >
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; } +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; + + loop_until_timeout(timeout, read_time) { + ret = regmap_bulk_read(regmap, offset, buf, count); + dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", + count, offset, ret, jiffies); + if (!ret) + return count; + } + + return -ETIMEDOUT; +} + static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf, unsigned int offset, size_t count) { @@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count) while (count) { int status; - status = at24->read_func(at24, buf, off, count); + status = at24_regmap_read(at24, buf, off, count); if (status < 0) { mutex_unlock(&at24->lock); pm_runtime_put(&client->dev); @@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip) } } +static unsigned int at24_get_offset_adj(u8 flags) +{ + if (flags & AT24_FLAG_MAC) + return 0x90; + else if (flags & AT24_FLAG_SERIAL && flags & AT24_FLAG_ADDR16) + /* + * For 16 bit address pointers, the word address must contain + * a '10' sequence in bits 11 and 10 regardless of the + * intended position of the address pointer. + */ + return BIT(11); + else if (flags & AT24_FLAG_SERIAL) + /* + * Otherwise the word address must begin with a '10' sequence, + * regardless of the intended address. + */ + return BIT(7); + else + return 0; +} + static const struct regmap_config regmap_config_8 = { .reg_bits = 8, .val_bits = 8, @@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) return -EINVAL; } + at24->offset_adj = at24_get_offset_adj(chip.flags); + if (chip.flags & AT24_FLAG_SERIAL) { at24->read_func = at24_eeprom_read_serial; } else if (chip.flags & AT24_FLAG_MAC) {
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@gmail.com> --- 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(-)