Re: [flashrom] [PATCH] fix output for multiple found flash chips

[Stefan Tauner]

On Fri, 24 Jun 2011 19:03:52 +0200
Stefan Tauner  wrote:
> missing quotes
> missing commas
> Signed-off-by: Stefan Tauner
>
> cli_classic.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/cli_classic.c b/cli_classic.c
> index 59096a0..aba6edf 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -373,9 +373,10 @@ int cli_classic(int argc, char *argv[])
>    }
>
> if (chipcount > 1) {
> printf("Multiple flash chips were detected:");
> for (i = 0; i < chipcount; i++)
> 	printf(" %s", flashes[i].name);
> printf("Multiple flash chips were detected: \"%s\"",
> 	flashes[0].name);
> for (i = 1; i < chipcount; i++)
> 	printf(", \"%s\"", flashes[i].name);
> printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> programmer_shutdown();
> exit(1);

since no one else has reviewed this yet and it is fairly trivial, i will
add it with my next "tested stuff" commit

[PATCH] fix output for multiple found flash chips

[Carl-Daniel Hailfinger]

Am 25.07.2011 00:32 schrieb Stefan Tauner:
> On Fri, 24 Jun 2011 19:03:52 +0200 Stefan Tauner wrote:
> > missing quotes
> > missing commas
> > Signed-off-by: Stefan Tauner
> >
> > cli_classic.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/cli_classic.c b/cli_classic.c
> > index 59096a0..aba6edf 100644
> > --- a/cli_classic.c
> > +++ b/cli_classic.c
> > @@ -373,9 +373,10 @@ int cli_classic(int argc, char *argv[])
> >    }
> >
> > if (chipcount > 1) {
> > printf("Multiple flash chips were detected:");
> > for (i = 0; i < chipcount; i++)
> > 	printf(" %s", flashes[i].name);
> > printf("Multiple flash chips were detected: \"%s\"",
> > 	flashes[0].name);
> > for (i = 1; i < chipcount; i++)
> > 	printf(", \"%s\"", flashes[i].name);
> > printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> > programmer_shutdown();
> > exit(1);
>
> since no one else has reviewed this yet and it is fairly trivial, i will
> add it with my next "tested stuff" commit

I'm not sure if this is a good idea. People might think that the chip
name contains quotes. Then again, with chip names containing commas and
other funny characters, it might even help.

Regards,
Carl-Daniel

[PATCH] fix output for multiple found flash chips

[Stefan Tauner]

On Mon, 25 Jul 2011 01:31:06 +0200
Carl-Daniel Hailfinger  wrote:
> Am 25.07.2011 00:32 schrieb Stefan Tauner:
> > On Fri, 24 Jun 2011 19:03:52 +0200 Stefan Tauner wrote:
> > > missing quotes
> > > missing commas
> > > Signed-off-by: Stefan Tauner
> > >
> > > cli_classic.c |    7 ++++---
> > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/cli_classic.c b/cli_classic.c
> > > index 59096a0..aba6edf 100644
> > > --- a/cli_classic.c
> > > +++ b/cli_classic.c
> > > @@ -373,9 +373,10 @@ int cli_classic(int argc, char *argv[])
> > >    }
> > >
> > > if (chipcount > 1) {
> > > printf("Multiple flash chips were detected:");
> > > for (i = 0; i < chipcount; i++)
> > > 	printf(" %s", flashes[i].name);
> > > printf("Multiple flash chips were detected: \"%s\"",
> > > 	flashes[0].name);
> > > for (i = 1; i < chipcount; i++)
> > > 	printf(", \"%s\"", flashes[i].name);
> > > printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> > > programmer_shutdown();
> > > exit(1);
> >
> > since no one else has reviewed this yet and it is fairly trivial, i will
> > add it with my next "tested stuff" commit
>
> I'm not sure if this is a good idea. People might think that the chip
> name contains quotes. Then again, with chip names containing commas and
> other funny characters, it might even help.

i think our output when we find a chip is much more misleading:
> Found chip "Winbond W25X64"

if one tries to use -c "Winbond W25X64" then it will fail, the code
above has no such problem. i am certain some termination between
multiple chips is needed, even if there are none with spaces in their
names. this increases readability of the message a lot imho:
Multiple flash chips were detected: ABC/DEF XYZ
vs.
Multiple flash chips were detected: "ABC/DEF", "XYZ"

[PATCH] fix output for multiple found flash chips

[Carl-Daniel Hailfinger]

Am 25.07.2011 02:06 schrieb Stefan Tauner:
> On Mon, 25 Jul 2011 01:31:06 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 25.07.2011 00:32 schrieb Stefan Tauner:
> > > On Fri, 24 Jun 2011 19:03:52 +0200 Stefan Tauner wrote:
> > > > missing quotes
> > > > missing commas
> > > > Signed-off-by: Stefan Tauner
> > > >
> > > > cli_classic.c |    7 ++++---
> > > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/cli_classic.c b/cli_classic.c
> > > > index 59096a0..aba6edf 100644
> > > > --- a/cli_classic.c
> > > > +++ b/cli_classic.c
> > > > @@ -373,9 +373,10 @@ int cli_classic(int argc, char *argv[])
> > > >    }
> > > >
> > > > if (chipcount > 1) {
> > > > printf("Multiple flash chips were detected:");
> > > > for (i = 0; i < chipcount; i++)
> > > > 	printf(" %s", flashes[i].name);
> > > > printf("Multiple flash chips were detected: \"%s\"",
> > > > 	flashes[0].name);
> > > > for (i = 1; i < chipcount; i++)
> > > > 	printf(", \"%s\"", flashes[i].name);
> > > > printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> > > > programmer_shutdown();
> > > > exit(1);
> > >
> > > since no one else has reviewed this yet and it is fairly trivial, i will
> > > add it with my next "tested stuff" commit
> >
> > I'm not sure if this is a good idea. People might think that the chip
> > name contains quotes. Then again, with chip names containing commas and
> > other funny characters, it might even help.
>
> i think our output when we find a chip is much more misleading:
> > Found chip "Winbond W25X64"
>
> if one tries to use -c "Winbond W25X64" then it will fail, the code

Ouch, yes indeed. Would it make sense to remove the quotes there?
> above has no such problem. i am certain some termination between
> multiple chips is needed, even if there are none with spaces in their
> names. this increases readability of the message a lot imho:
> Multiple flash chips were detected: ABC/DEF XYZ
> vs.
> Multiple flash chips were detected: "ABC/DEF", "XYZ"

Agreed, go ahead. With or without a quote change for the "found chip"
output, this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[PATCH] fix output for multiple found flash chips

[Stefan Tauner]

On Mon, 25 Jul 2011 08:51:08 +0200
Carl-Daniel Hailfinger  wrote:
> Am 25.07.2011 02:06 schrieb Stefan Tauner:
> > On Mon, 25 Jul 2011 01:31:06 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 25.07.2011 00:32 schrieb Stefan Tauner:
> >
> > i think our output when we find a chip is much more misleading:
> > > Found chip "Winbond W25X64"
> >
> > if one tries to use -c "Winbond W25X64" then it will fail, the code
>
> Ouch, yes indeed. Would it make sense to remove the quotes there?

my gut feel tells me that we should handle this consistently everywhere.
also removing the quotes completely does not fully remove the source of
misunderstanding.
the problem is that we call the chip by vendor+model here, but not in
the (documentation of) -c parameter. there are various possibilities to
mitigate that. the best thing i can think of is rephrasing the "found"
message completely and having quotes in the right places like in the
following:

Found chip "W25X64" from Winbond
or
Found a Winbond chip "W25X64"
or similar, i.e. combining "chip" with the model/type only and not the
vendor (+ quoting the chip name in the case of special characters in
the name).
> > above has no such problem. i am certain some termination between
> > multiple chips is needed, even if there are none with spaces in their
> > names. this increases readability of the message a lot imho:
> > Multiple flash chips were detected: ABC/DEF XYZ
> > vs.
> > Multiple flash chips were detected: "ABC/DEF", "XYZ"
>
> Agreed, go ahead. With or without a quote change for the "found chip"
> output, this is
> Acked-by: Carl-Daniel Hailfinger

ok thank you. i would like to hear your opinion on the above first
though :)

[PATCH] fix output for multiple found flash chips

[Carl-Daniel Hailfinger]

Am 25.07.2011 13:37 schrieb Stefan Tauner:
> On Mon, 25 Jul 2011 08:51:08 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 25.07.2011 02:06 schrieb Stefan Tauner:
> > > On Mon, 25 Jul 2011 01:31:06 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Am 25.07.2011 00:32 schrieb Stefan Tauner:
> > >
> > > i think our output when we find a chip is much more misleading:
> > > > Found chip "Winbond W25X64"
> > >
> > > if one tries to use -c "Winbond W25X64" then it will fail, the code
> >
> > Ouch, yes indeed. Would it make sense to remove the quotes there?
>
> my gut feel tells me that we should handle this consistently everywhere.
> also removing the quotes completely does not fully remove the source of
> misunderstanding.
> the problem is that we call the chip by vendor+model here, but not in
> the (documentation of) -c parameter. there are various possibilities to
> mitigate that. the best thing i can think of is rephrasing the "found"
> message completely and having quotes in the right places like in the
> following:
>
> Found chip "W25X64" from Winbond
> or
> Found a Winbond chip "W25X64"
> or similar, i.e. combining "chip" with the model/type only and not the
> vendor (+ quoting the chip name in the case of special characters in
> the name).

Found Winbond flash chip "W25X64"
> > > above has no such problem. i am certain some termination between
> > > multiple chips is needed, even if there are none with spaces in their
> > > names. this increases readability of the message a lot imho:
> > > Multiple flash chips were detected: ABC/DEF XYZ
> > > vs.
> > > Multiple flash chips were detected: "ABC/DEF", "XYZ"
> >
> > Agreed, go ahead. With or without a quote change for the "found chip"
> > output, this is
> > Acked-by: Carl-Daniel Hailfinger
>
> ok thank you. i would like to hear your opinion on the above first
> though :)

See above.

Regards,
Carl-Daniel

[PATCH] fix output for multiple found flash chips

[Stefan Tauner]

On Mon, 25 Jul 2011 21:58:36 +0200
Carl-Daniel Hailfinger  wrote:
> Found Winbond flash chip "W25X64"

ok, committed with the tested stuff commit #5 in r1386