[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[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.

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 <abrodkin@synopsys.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
Cc: Heiko Schocher <hs@denx.de>
---
 common/cmd_eeprom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
index 6eab1ea..571240a 100644
--- a/common/cmd_eeprom.c
+++ b/common/cmd_eeprom.c
@@ -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;
-- 
2.4.3

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Marek Vasut]

On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote:
> 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 <abrodkin@synopsys.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Heiko Schocher <hs@denx.de>

Obviously correct,

Acked-by: Marek Vasut <marex@denx.de>

Thanks for spotting this, nice!

Best regards,
Marek Vasut

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Alexey Brodkin]

Hi Marek,

On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote:
> On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote:
> > 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 <abrodkin@synopsys.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Heiko Schocher <hs@denx.de>
> 
> Obviously correct,
> 
> Acked-by: Marek Vasut <marex@denx.de>
> 
> 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

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Marek Vasut]

On Tuesday, December 15, 2015 at 09:07:01 AM, Alexey Brodkin wrote:
> Hi Marek,
> 
> On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote:
> > On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote:
> > > 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 <abrodkin@synopsys.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Simon Glass <sjg@chromium.org>
> > > Cc: Tom Rini <trini@konsulko.com>
> > > Cc: Heiko Schocher <hs@denx.de>
> > 
> > Obviously correct,
> > 
> > Acked-by: Marek Vasut <marex@denx.de>
> > 
> > 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.

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

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

:)

Best regards,
Marek Vasut

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Alexey Brodkin]

Hi Marek,

On Tue, 2015-12-15 at 12:47 +0100, Marek Vasut wrote:
> On Tuesday, December 15, 2015 at 09:07:01 AM, Alexey Brodkin wrote:
> > Hi Marek,
> > 
> > On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote:
> > > On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote:
> > > > 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 <abrodkin@synopsys.com>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Tom Rini <trini@konsulko.com>
> > > > Cc: Heiko Schocher <hs@denx.de>
> > > 
> > > Obviously correct,
> > > 
> > > Acked-by: Marek Vasut <marex@denx.de>
> > > 
> > > 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.
> 
> ... 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

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Tom Rini]

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

[snip]
> 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.

> 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.

> 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 :)

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'.

-- 
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>

[U-Boot] [PATCH] eeprom: fix eeprom write procedure

[Tom Rini]

On Mon, Dec 14, 2015 at 06:45:34PM +0300, Alexey Brodkin wrote:

> This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:

Applied to u-boot/master, thanks!

-- 
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>