eeprom: fix eeprom write procedure

PATCH New Alexey Brodkin 3 participants 7 messages Dec 14, 2015 – Dec 16, 2015

Share & tools

Alexey Brodkin

author

2015-12-14 15:45 UTC #0 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
----------------------->8--------------------
eeprom: Pull out the RW loop

Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8--------------------

where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow.

That lead to a missing delay in case of writing which lead to write failure (in my case no data was written).

Signed-off-by Alexey Brodkin <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Simon Glass <[email protected]>

Cc Tom Rini <[email protected]>

Cc Heiko Schocher <[email protected]>

---
 common/cmd_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/cmd_eeprom.c +1 -1

index 6eab1ea..571240a 100644
@@ -197,7 +197,7 @@ int eeprom_write(unsigned dev_addr, unsigned offset,
 	 * We must write the address again when changing pages
 	 * because the address counter only increments within a page.
 	 */
-	ret = eeprom_rw(dev_addr, offset, buffer, cnt, 1);
+	ret = eeprom_rw(dev_addr, offset, buffer, cnt, 0);
 
 	eeprom_write_enable(dev_addr, 0);
 	return ret;

Signature · 1 line

2.4.3

Marek Vasut

2015-12-15 00:27 UTC #1 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

Quoted 13 lines · Alexey Brodkin

This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
----------------------->8--------------------
eeprom: Pull out the RW loop
Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8--------------------
where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow.

Quoted 8 lines

That lead to a missing delay in case of writing which lead to write failure (in my case no data was written).
Signed-off-by Alexey Brodkin <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
Cc Heiko Schocher <[email protected]>

Obviously correct,

Acked-by Marek Vasut <[email protected]>

Thanks for spotting this, nice!

Best regards,
Marek Vasut

Alexey Brodkin

author

2015-12-15 08:07 UTC #2 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

Hi Marek,

Quoted 23 lines · Marek Vasut

Quoted 13 lines · Alexey Brodkin
This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
----------------------->8--------------------
eeprom: Pull out the RW loop
Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8--------------------
where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow.
Quoted 8 lines
That lead to a missing delay in case of writing which lead to write failure (in my case no data was written).
Signed-off-by Alexey Brodkin <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
Cc Heiko Schocher <[email protected]>

Quoted 5 lines

Obviously correct,
Acked-by Marek Vasut <[email protected]>
Thanks for spotting this, nice!

That was a nice exercise for me.
From the first glance DW SPI and ARC-specific changes were not guilty so I tried some previous RC-s and found that v2016.01-rc1 is good while rc2 is not.

So I recalled articles and talks about git bisect. And literally in few next minutes I knew commit that introduced that breakage. At that point problem became really obvious.

That said it's really fantastic what cool tools we have now that simplify our life as developers significantly.

-Alexey

Marek Vasut

2015-12-15 11:47 UTC #3 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

Quoted 32 lines · Alexey Brodkin

Hi Marek,
Quoted 23 lines · Marek Vasut
Quoted 13 lines · Alexey Brodkin
This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
----------------------->8--------------------
eeprom: Pull out the RW loop
Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8--------------------
where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow.
Quoted 8 lines
That lead to a missing delay in case of writing which lead to write failure (in my case no data was written).
Signed-off-by Alexey Brodkin <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
Cc Heiko Schocher <[email protected]>
Quoted 5 lines
Obviously correct,
Acked-by Marek Vasut <[email protected]>
Thanks for spotting this, nice!

Quoted 8 lines

That was a nice exercise for me.
From the first glance DW SPI and ARC-specific changes were not guilty so I tried some previous RC-s and found that v2016.01-rc1 is good while rc2 is not.
So I recalled articles and talks about git bisect. And literally in few next minutes I knew commit that introduced that breakage. At that point problem became really obvious.

... and then you cursed at me, yeah, I did not sleep very well last night ;-)

Quoted 2 lines

That said it's really fantastic what cool tools we have now that simplify our life as developers significantly.

Best regards,
Marek Vasut

Alexey Brodkin

author

2015-12-15 12:09 UTC #4 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

Hi Marek,

Quoted 38 lines · Marek Vasut

Quoted 37 lines · Alexey Brodkin
Hi Marek,
Quoted 23 lines · Marek Vasut
Quoted 13 lines · Alexey Brodkin
This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
----------------------->8--------------------
eeprom: Pull out the RW loop
Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8--------------------
where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow.
Quoted 8 lines
That lead to a missing delay in case of writing which lead to write failure (in my case no data was written).
Signed-off-by Alexey Brodkin <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
Cc Heiko Schocher <[email protected]>
Quoted 5 lines
Obviously correct,
Acked-by Marek Vasut <[email protected]>
Thanks for spotting this, nice!
That was a nice exercise for me.
From the first glance DW SPI and ARC-specific changes were not guilty so I tried some previous RC-s and found that v2016.01-rc1 is good while rc2 is not.

Quoted 5 lines

Quoted 3 lines
So I recalled articles and talks about git bisect. And literally in few next minutes I knew commit that introduced that breakage. At that point problem became really obvious.
... and then you cursed at me, yeah, I did not sleep very well last night ;-)

Well 1 beer may definitely help to improve our relationships indeed :)

But jokes aside I'm a bit confused with development cycle of U-Boot. I mean here we're free to push quite important changes at any point even if tomorrow Tom is cutting the next release. RCs are out of the question at all.

I have to confess I do it myself from time to time just because it's so easy and nobody cares. But that really makes me worry because I cannot afford running U-Boot on every board I support on each and every commit. Which in turn means there's a possibility in final release some of my boards will be broken to some extent.

That said I really like Linux development procedure when merge window is open just for a week or so and then only regression fixes are accepted during RC cycles.

Still that very-very formal approach could be a bit of overkill for us here. But there's another good example - Buildroot.

In the same way as we do it they accept patches right in master branch but only until the first RC. From RC1 on only fixes go in master, everything else goes to "next" branch. And "next" gets merged in master right after release.

Again I'm not following U-boot mailing list closely so I might be missing similar ongoing discussion so please be lenient to that my rant :)

-Alexey

Tom Rini

2015-12-15 23:36 UTC #5 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

On Tue, Dec 15, 2015 at 12:09:21PM +0000, Alexey Brodkin wrote:

[snip]

Quoted 4 lines

But jokes aside I'm a bit confused with development cycle of U-Boot. I mean here we're free to push quite important changes at any point even if tomorrow Tom is cutting the next release. RCs are out of the question at all.

Well, the rule of thumb is to get changes out by "around" rc1. I did -rc1 on the 16th, Marek posted this series on the 10th and I merged it on the 21st. So all things considered, I know I've done worse merges.

But the topic also does come up more often than it should, so something in the balance isn't right.

Quoted 5 lines

I have to confess I do it myself from time to time just because it's so easy and nobody cares. But that really makes me worry because I cannot afford running U-Boot on every board I support on each and every commit. Which in turn means there's a possibility in final release some of my boards will be broken to some extent.

Well, that's just it. I put a good level of discretion to the various area custodians to take what they're comfortable with, given what they can test and how close to the release things are.

Quoted 7 lines

That said I really like Linux development procedure when merge window is open just for a week or so and then only regression fixes are accepted during RC cycles.
Still that very-very formal approach could be a bit of overkill for us here. But there's another good example - Buildroot.

Quoted 7 lines

In the same way as we do it they accept patches right in master branch but only until the first RC. From RC1 on only fixes go in master, everything else goes to "next" branch. And "next" gets merged in master right after release.
Again I'm not following U-boot mailing list closely so I might be missing similar ongoing discussion so please be lenient to that my rant :)

Well, what I don't like about 'next' is people are either going to be testing 'next' (And not what's going to be released soon) or they're testing master (and we're just delaying problems being seen and mildly more annoying to bisect back to).

At ELC-E we had talked about moving to a nominal 2 month rather than 3 month (meaning it's released when it's ready, not a hard calendar deadline) release schedule. That would remove some of the pressure of 'take $X so it's not seemed to be forgotten between releases'.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151215/e913a187/attachment.sig>

Tom Rini

2015-12-16 15:31 UTC #6 lore raw

PATCH 1/1 eeprom: fix eeprom write procedure

In reply to Alexey Brodkin

Quoted 1 line · Alexey Brodkin

This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151216/9cc38993/attachment.sig>