Patchwork [1/3] 128 bytes write granularity support

login
register
about
Submitter Paul Kocialkowski
Date 2015-10-10 14:20:19
Message ID <1444486821-10619-1-git-send-email-contact@paulk.fr>
Download mbox | patch
Permalink /patch/4323/
State Accepted
Headers show

Comments

Paul Kocialkowski - 2015-10-10 14:20:19
Some chips such as the ENE KB9012 internal flash require a write granularity of
128 bytes.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 flash.h    | 1 +
 flashrom.c | 6 ++++++
 2 files changed, 7 insertions(+)
Nico Huber - 2015-10-14 19:43:04
On 10.10.2015 16:20, Paul Kocialkowski wrote:
> Some chips such as the ENE KB9012 internal flash require a write granularity of
> 128 bytes.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
clean patch, +2

> ---
>  flash.h    | 1 +
>  flashrom.c | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/flash.h b/flash.h
> index 2c2839f..24861ba 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -85,6 +85,7 @@ enum write_granularity {
>  	write_gran_1bit,	/* Each bit can be cleared individually. */
>  	write_gran_1byte,	/* A byte can be written once. Further writes to an already written byte cause
>  				 * its contents to be either undefined or to stay unchanged. */
> +	write_gran_128bytes,	/* If less than 128 bytes are written, the unwritten bytes are undefined. */
>  	write_gran_264bytes,	/* If less than 264 bytes are written, the unwritten bytes are undefined. */
>  	write_gran_512bytes,	/* If less than 512 bytes are written, the unwritten bytes are undefined. */
>  	write_gran_528bytes,	/* If less than 528 bytes are written, the unwritten bytes are undefined. */
> diff --git a/flashrom.c b/flashrom.c
> index d51a44c..c9c7e31 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -781,6 +781,9 @@ int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum
>  				break;
>  			}
>  		break;
> +	case write_gran_128bytes:
> +		result = need_erase_gran_bytes(have, want, len, 128);
> +		break;
>  	case write_gran_256bytes:
>  		result = need_erase_gran_bytes(have, want, len, 256);
>  		break;
> @@ -847,6 +850,9 @@ static unsigned int get_next_write(const uint8_t *have, const uint8_t *want, uns
>  	case write_gran_1byte_implicit_erase:
>  		stride = 1;
>  		break;
> +	case write_gran_128bytes:
> +		stride = 128;
> +		break;
>  	case write_gran_256bytes:
>  		stride = 256;
>  		break;
>
Stefan Tauner - 2015-10-16 02:25:34
On Wed, 14 Oct 2015 21:43:04 +0200
Nico Huber <nico.h@gmx.de> wrote:

> On 10.10.2015 16:20, Paul Kocialkowski wrote:
> > Some chips such as the ENE KB9012 internal flash require a write granularity of
> > 128 bytes.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> clean patch, +2

I've understood that as an acked-by... ;)
Thanks to both of you.
Committed in r1897.
Paul Kocialkowski - 2015-10-18 17:37:35
Hi,

Le vendredi 16 octobre 2015 à 04:25 +0200, Stefan Tauner a écrit :
> On Wed, 14 Oct 2015 21:43:04 +0200
> Nico Huber <nico.h@gmx.de> wrote:
> 
> > On 10.10.2015 16:20, Paul Kocialkowski wrote:
> > > Some chips such as the ENE KB9012 internal flash require a write
> > > granularity of
> > > 128 bytes.
> > > 
> > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > clean patch, +2
> 
> I've understood that as an acked-by... ;)
> Thanks to both of you.
> Committed in r1897.

Great, thanks a lot Nico for reviewing this and Stefan for merging it!

I noticed that you have reworded the commit message: I usually do not
use conjugated verbs in commit headline. Does this conflict with
flashrom's guidelines?

It looks a bit strange to me to have my commit worded in a way that
really does not match what I naturally do!
Carl-Daniel Hailfinger - 2015-10-18 18:58:34
Hi Paul,

On 18.10.2015 19:37, Paul Kocialkowski wrote:
> I noticed that you have reworded the commit message: I usually do not > use conjugated verbs in commit headline. Does this conflict with >
flashrom's guidelines?

No formal guidelines, but "128 bytes write granularity support" is
missing a crucial piece of information: Was this a bugfix, a new feature
or something you removed?
The reworded commit message has that info.

> It looks a bit strange to me to have my commit worded in a way that > really does not match what I naturally do!

Regards,
Carl-Daniel
Paul Kocialkowski - 2015-10-22 06:58:53
Hi Carl-Daniel,

Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a
écrit :
> On 18.10.2015 19:37, Paul Kocialkowski wrote:
> > I noticed that you have reworded the commit message: I usually do
> > not > use conjugated verbs in commit headline. Does this conflict
> > with >
> flashrom's guidelines?
> 
> No formal guidelines, but "128 bytes write granularity support" is
> missing a crucial piece of information: Was this a bugfix, a new
> feature
> or something you removed?
> The reworded commit message has that info.

Well, it does end with "support", which indicates that support for this
was introduced, so I don't think it was confusing.

I really see commit headlines as titles, not active phrases and would
like to keep my commits phrased this way.

Please let me know during patch review next time you find the commit
headline not precise enough.

Thanks!
Stefan Tauner - 2015-10-22 20:05:00
On Thu, 22 Oct 2015 08:58:53 +0200
Paul Kocialkowski <contact@paulk.fr> wrote:

> Hi Carl-Daniel,
> 
> Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a
> écrit :
> > On 18.10.2015 19:37, Paul Kocialkowski wrote:
> > > I noticed that you have reworded the commit message: I usually do
> > > not > use conjugated verbs in commit headline. Does this conflict
> > > with >
> > flashrom's guidelines?
> > 
> > No formal guidelines, but "128 bytes write granularity support" is
> > missing a crucial piece of information: Was this a bugfix, a new
> > feature
> > or something you removed?
> > The reworded commit message has that info.
> 
> Well, it does end with "support", which indicates that support for this
> was introduced, so I don't think it was confusing.
> 
> I really see commit headlines as titles, not active phrases and would
> like to keep my commits phrased this way.
> 
> Please let me know during patch review next time you find the commit
> headline not precise enough.

Hello Paul,

I have not replied yet because Carl-Daniel's message described the
thoughts I had/have when changing commit logs pretty much exactly.
Your approach on commit logs (seeing the first line as the "subject") is
IMHO a defensible argument. After all, that's how git calls it as well
in many places.

However, using imperative sentences is much more common and IMHO it is
better. I see every commit as an entity that changes our code base...
so it always *does* something and often it is interesting what the most
descriptive verb is for what it is doing (e.g. add, fix, improve,
refactor, ...) to better understand what the change is about. Just like
Carl-Daniel explained for the commit in question.

I can not be sure that if we someone looks at the commit in three years
he will remember or even know that 128 byte granularity was introduced
about the time this commit happened. It is much easier to understand
a commit if the subject contains a verb that makes the action clear.

Also, let's suppose we would have to revert a commit because it was
really a bad idea to commit it in the first place and we can not fix it
easily. The normal commit message would be
«Revert "128 bytes write granularity support"» in your case vs.
«Revert "Add support for 128 bytes write granularity.» in the
imperative case. The issue with the subject without a verb is similar
to the original. In the second case it is completely clear that the
support was removed again. Without a verb it could be a fix that is
reverted, an improvement, or even the revert of removing the support...
it is simply not explicit enough IMHO.

Others have written about the subject as well, but I can not find a lot
of texts explaining the WHY very good... this is one example:

http://chris.beams.io/posts/git-commit/#imperative

Your other patches are affected as well and I intend to rephrase them
as I see fit. I hope you understand/agree now... if not I'll have
another argument: the maintainers have to maintain the codebase and are
the ones who read commit messages the most, hence it should be the
decision of the maintainers what's written in commit messages because
they are affected most by them. The submitter can simply dump the code
and hope it gets merged but apart from seeing some mails with the
subject during communication it does not affect his work on the project
later (because there often is none... otherwise he is not merely a
submitter).
Paul Kocialkowski - 2015-10-27 12:04:00
Hi Stefan,

Le jeudi 22 octobre 2015 à 22:05 +0200, Stefan Tauner a écrit :
> On Thu, 22 Oct 2015 08:58:53 +0200
> Paul Kocialkowski <contact@paulk.fr> wrote:
> 
> > Hi Carl-Daniel,
> > 
> > Le dimanche 18 octobre 2015 à 20:58 +0200, Carl-Daniel Hailfinger a
> > écrit :
> > > On 18.10.2015 19:37, Paul Kocialkowski wrote:
> > > > I noticed that you have reworded the commit message: I usually do
> > > > not > use conjugated verbs in commit headline. Does this conflict
> > > > with >
> > > flashrom's guidelines?
> > > 
> > > No formal guidelines, but "128 bytes write granularity support" is
> > > missing a crucial piece of information: Was this a bugfix, a new
> > > feature
> > > or something you removed?
> > > The reworded commit message has that info.
> > 
> > Well, it does end with "support", which indicates that support for this
> > was introduced, so I don't think it was confusing.
> > 
> > I really see commit headlines as titles, not active phrases and would
> > like to keep my commits phrased this way.
> > 
> > Please let me know during patch review next time you find the commit
> > headline not precise enough.
> 
> Hello Paul,
> 
> I have not replied yet because Carl-Daniel's message described the
> thoughts I had/have when changing commit logs pretty much exactly.
> Your approach on commit logs (seeing the first line as the "subject") is
> IMHO a defensible argument. After all, that's how git calls it as well
> in many places.
> 
> However, using imperative sentences is much more common and IMHO it is
> better. I see every commit as an entity that changes our code base...
> so it always *does* something and often it is interesting what the most
> descriptive verb is for what it is doing (e.g. add, fix, improve,
> refactor, ...) to better understand what the change is about. Just like
> Carl-Daniel explained for the commit in question.

Well, I don't see both points of view as contradictory. When introducing
support for something new, I think we're not losing any information when
formatting the subject line as:
Feature foo support introduction
instead of
Introduce support for feature foo

This goes for all the things that we might want to describe:
* Bug #123 fixup
* Feature bar improvement
* Baz code re-factoring

Most active verbs have (more or less direct) equivalent nouns that can
be used.

I get the argument that we're doing something to the code base, so it
makes sense to use active verbs, but for some reason, I just find that
somewhat ugly. Perhaps my vision of things will change with time, but
that's how I see it for now.

Also, it seems that there is a lot of disparity regarding the tense used
for such verbs : Added support for X/Add support for X/Adds support for
X. And I don't really see any good way to choose between those. And I
hate that sort of lack of consistency, so using non-verbal sentences
felt like an elegant solution.

> I can not be sure that if we someone looks at the commit in three years
> he will remember or even know that 128 byte granularity was introduced
> about the time this commit happened. It is much easier to understand
> a commit if the subject contains a verb that makes the action clear.

Well, let's not forget that commit messages come with longer
explanations that must play a role, more than saying the same as the
subject. I really see the subject as a title to the longer commit
message.

> Also, let's suppose we would have to revert a commit because it was
> really a bad idea to commit it in the first place and we can not fix it
> easily. The normal commit message would be
> «Revert "128 bytes write granularity support"» in your case vs.
> «Revert "Add support for 128 bytes write granularity.» in the
> imperative case. The issue with the subject without a verb is similar
> to the original. In the second case it is completely clear that the
> support was removed again. Without a verb it could be a fix that is
> reverted, an improvement, or even the revert of removing the support...
> it is simply not explicit enough IMHO.

Well, I think that "support" implies "support introduction", but that's
something else. Also, git wraps the original line around quotation
marks, which also adds some clarity.

> Others have written about the subject as well, but I can not find a lot
> of texts explaining the WHY very good... this is one example:
> 
> http://chris.beams.io/posts/git-commit/#imperative

One very good point here is that git uses the imperative when creating a
commit on its own (Revert, Merge, etc). That sounds reasonable enough as
a guideline for me to switch my behaviour.

> Your other patches are affected as well and I intend to rephrase them
> as I see fit. I hope you understand/agree now... if not I'll have
> another argument: the maintainers have to maintain the codebase and are
> the ones who read commit messages the most, hence it should be the
> decision of the maintainers what's written in commit messages because
> they are affected most by them. The submitter can simply dump the code
> and hope it gets merged but apart from seeing some mails with the
> subject during communication it does not affect his work on the project
> later (because there often is none... otherwise he is not merely a
> submitter).

Well, that makes sense, yes, but keep in mind that it's also part of the
author's contribution, so I'd like things to be discussed beforehand,
just like it is somewhat rude to change a patch the way you like and
merge it without discussing it with its author through code review
first.

Let's say that I wouldn't object to having commit subjects rephrased the
way git does it internally.

Thanks for taking the time to explain all this.

Paul Kocialkowski

Patch

diff --git a/flash.h b/flash.h
index 2c2839f..24861ba 100644
--- a/flash.h
+++ b/flash.h
@@ -85,6 +85,7 @@  enum write_granularity {
 	write_gran_1bit,	/* Each bit can be cleared individually. */
 	write_gran_1byte,	/* A byte can be written once. Further writes to an already written byte cause
 				 * its contents to be either undefined or to stay unchanged. */
+	write_gran_128bytes,	/* If less than 128 bytes are written, the unwritten bytes are undefined. */
 	write_gran_264bytes,	/* If less than 264 bytes are written, the unwritten bytes are undefined. */
 	write_gran_512bytes,	/* If less than 512 bytes are written, the unwritten bytes are undefined. */
 	write_gran_528bytes,	/* If less than 528 bytes are written, the unwritten bytes are undefined. */
diff --git a/flashrom.c b/flashrom.c
index d51a44c..c9c7e31 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -781,6 +781,9 @@  int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum
 				break;
 			}
 		break;
+	case write_gran_128bytes:
+		result = need_erase_gran_bytes(have, want, len, 128);
+		break;
 	case write_gran_256bytes:
 		result = need_erase_gran_bytes(have, want, len, 256);
 		break;
@@ -847,6 +850,9 @@  static unsigned int get_next_write(const uint8_t *have, const uint8_t *want, uns
 	case write_gran_1byte_implicit_erase:
 		stride = 1;
 		break;
+	case write_gran_128bytes:
+		stride = 128;
+		break;
 	case write_gran_256bytes:
 		stride = 256;
 		break;