Re: [flashrom] [PATCH 1/3] serprog: add SPI support

[Uwe Hermann]

On Thu, Sep 08, 2011 at 12:56:21AM +0200, Stefan Tauner wrote:
> +	0x13 (O_SPIOP):
> +		Send and receive bytes via SPI.
> +		Maximum slen is Q_WRNMAXLEN result after Q_BUSTYPE returns
>                                                                           ^
>                                                                       Full stop.
> +		only SPI or S_BUSTYPE == SPI is used. Same for rlen and Q_RDNMAXLEN.
>                 ^
>              Capitalize
> +static struct spi_programmer spi_programmer_serprog = {
> +	.type = SPI_CONTROLLER_SERPROG,
> +	.max_data_read = MAX_DATA_READ_UNLIMITED,
> +	.max_data_write = MAX_DATA_WRITE_UNLIMITED,
> +	.command = serprog_spi_send_command,
> +	.multicommand = default_spi_send_multicommand,
> +	.read = serprog_spi_read,
> +	.write_256 = default_spi_write_256,
> +};

Please use the "aligned" style using tabs, see e.g.

  "static const struct spi_programmer spi_programmer_linux"

in linux_spi.c.
> +	if (sp_docommand(S_CMD_Q_BUSTYPE, 0, NULL, 1, &c)) {
> +		msg_perr("Warning: NAK to query supported buses\n");

perr and "Warning" doesn't sound consistent, but can be fixed in
another patch.
> +		/* Success of any of these commands is optional. We dont need

dont -> don't
> +		   the programmer to tell us it's limits, but if it doesnt, we

it's -> its
doesnt -> doesn't
> +		if (!sp_docommand(S_CMD_Q_RDNMAXLEN, 0, NULL, 3, rbuf)) {
> +			uint32_t v;
> +			v = ((unsigned int)(rbuf[0]) << 0);
> +			v |= ((unsigned int)(rbuf[1]) << 8);
> +			v |= ((unsigned int)(rbuf[2]) << 16);

Why (unsigned int) and not (uint32_t)? This is done in multiple places.
> +		/* This could be translated to single byte reads (if missing),
> +		 * but now we dont support that. */

dont -> don't
> +	if (!sp_check_commandavail(S_CMD_O_DELAY)) {
> +		internal_delay(delay);
> +		msg_pdbg("Note: serprog_delay used, but the programmer doesnt support delay\n");

doesnt -> doesn't
> +	for(i = 0; i < len; ) {
>            ^
>         missing space

Otherwise
Acked-by: Uwe Hermann <[e-mail]>


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org

[PATCH 1/3] serprog: add SPI support

[Stefan Tauner]

On Thu, 15 Sep 2011 21:59:30 +0200
Uwe Hermann <[e-mail]> wrote:
> On Thu, Sep 08, 2011 at 12:56:21AM +0200, Stefan Tauner wrote:
> > +	0x13 (O_SPIOP):
> > +		Send and receive bytes via SPI.
> > +		Maximum slen is Q_WRNMAXLEN result after Q_BUSTYPE returns
> >                                                                           ^
> >                                                                       Full stop.
> > +		only SPI or S_BUSTYPE == SPI is used. Same for rlen and Q_RDNMAXLEN.
> >                 ^
> >              Capitalize

hm i think urja wanted this to be in one sentence otherwise it does not
make sense.
i have changed this now to
"Maximum slen is Q_WRNMAXLEN in case Q_BUSTYPE returns SPI only or
S_BUSTYPE was used to set SPI exclusively before. Same for rlen and Q_RDNMAXLEN."
> > +	if (sp_docommand(S_CMD_Q_BUSTYPE, 0, NULL, 1, &c)) {
> > +		msg_perr("Warning: NAK to query supported buses\n");
>
> perr and "Warning" doesn't sound consistent, but can be fixed in
> another patch.

yes, but it is used all over the place.
> > +		if (!sp_docommand(S_CMD_Q_RDNMAXLEN, 0, NULL, 3, rbuf)) {
> > +			uint32_t v;
> > +			v = ((unsigned int)(rbuf[0]) << 0);
> > +			v |= ((unsigned int)(rbuf[1]) << 8);
> > +			v |= ((unsigned int)(rbuf[2]) << 16);
>
> Why (unsigned int) and not (uint32_t)? This is done in multiple places.

i have no idea actually, i just moved the code :)
i have left it alone for now. this can be fixed later too.
> > +	if (!sp_check_commandavail(S_CMD_O_DELAY)) {
> > +		internal_delay(delay);
> > +		msg_pdbg("Note: serprog_delay used, but the programmer doesnt support delay\n");
>
> doesnt -> doesn't

and 80 char limit..
> > +	for(i = 0; i < len; ) {
> >            ^
> >         missing space

the missing third argument is a more WTF imho. especially since the
increment is done in the last line of the for body anyway... i have
moved it from there into the head of the for instruction.
> Otherwise
> Acked-by: Uwe Hermann <[e-mail]>

thanks for the review! i hope this is quite useful for some (future)
users. committed in r1442.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner