Re: [flashrom] [PATCH] do something useful with the dediprog LEDs

[Mathias Krause]

On 21.01.2011 20:09, Stefan Reinauer wrote:
> This patch makes use of the dediprog LEDs.

Nice!
> It would be nice to have some "more global" infrastructure for device
> dependent status reporting. Right now some scenarios can not be
> reported as an error, though the user would think of them as one. For
> example, if the chip is IDed successfully but not supported by
> flashrom, thus it can not be flashed. At best, the dediprog driver
> sees a successful RDID command, but has no knowledge about the failure
> to read/write the chip. Therefore it can not report it.
>
> However, the attached patch improves my work flow here quite a bit, so
> I thought it should be shared.

Thanks for the patch. Albeit it would be good to have a "noleds" option
to disable the functionality because we don't know if it breaks older
firmware versions.
> Implement proper LED handling for dediprog.
>
> ... so you don't have to stare at the screen to see when the write is 
> finished and whether it worked.
>
> Signed-off-by: Stefan Reinauer
> Index: dediprog.c
>
> --- dediprog.c	(revision 1254)
> +++ dediprog.c	(working copy)
> @@ -58,6 +58,33 @@
>
> //int usb_control_msg(usb_dev_handle *dev, int requesttype, int request, int value, int index, char *bytes, int size, int timeout);
>
> +/* Set/clear LEDs on dediprog. */
> +#define PASS_ON		(0 << 0)
> +#define PASS_OFF	(1 << 0)
> +#define BUSY_ON		(0 << 1)
> +#define BUSY_OFF	(1 << 1)
> +#define ERROR_ON	(0 << 2)
> +#define ERROR_OFF	(1 << 2)
> +static int current_led_status = 0;

Should be initialized to -1, see below why.
> +static int dediprog_set_leds(int leds)
> +{
> int ret;
> if (leds < 0 || leds > 7)
> leds = 0; // Bogus value, enable all LEDs

We can skip sending the USB command when the LED already has the value
we want it to. Something like:

if (leds == current_led_status)
    return 0;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, leds, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command Set LED 0x%x failed (%s)!\n", leds, usb_strerror());
> return 1;
> }
> current_led_status = leds;
> return 0;
>
> +}
>
> static int dediprog_set_spi_voltage(int millivolt)
>  {
>    int ret;
> @@ -211,20 +238,26 @@
>    int residue = start % chunksize ? chunksize - start % chunksize : 0;
>    int bulklen;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> if (residue) {
>   msg_pdbg("Slow read for partial block from 0x%x, length 0x%x\n",
>   	 start, residue);
>   ret = spi_read_chunked(flash, buf, start, residue, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> /* Round down. */
> bulklen = (len - residue) / chunksize * chunksize;
> ret = dediprog_spi_bulk_read(flash, buf + residue, start + residue,
> 		     bulklen);
> if (ret)
>
> if (ret) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return ret;
>
> }
>
> len -= residue + bulklen;
> if (len) {
>
> @@ -232,17 +265,29 @@
>    		 start, len);
>    	ret = spi_read_chunked(flash, buf + residue + bulklen,
>    			       start + residue + bulklen, len, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> int dediprog_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> int ret;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> /* No idea about the real limit. Maybe 12, maybe more, maybe less. */
> return spi_write_chunked(flash, buf, start, len, 12);
> ret = spi_write_chunked(flash, buf, start, len, 12);
> if (ret)
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> else
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);

Missing 'return ret;'
> }
>
> int dediprog_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> @@ -410,81 +455,8 @@
>    }
>    return 0;
>  }
>
> -/* Start/stop blinking?
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command J
> */
>
> -static int dediprog_command_g(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x03, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command G failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -/* Something.
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String
> Always followed by Command Set SPI Voltage nonzero
> */
>
> -static int dediprog_command_h(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x05, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command H failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>  #endif
>
> -/* Shutdown for firmware 5.x?
> This command swithces the "Pass" LED on.
> Present in all logs with firmware 5.1.5
> Often preceded by a SPI operation (Command Read SPI Bulk or Receive SPI)
> Always followed by Command Set SPI Voltage 0x0000
> */
>
> -static int dediprog_command_i(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x06, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command I failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -#if 0
> -/* Start/stop blinking?
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String on 5.1.5
> Always followed by Command Set SPI Voltage nonzero on 5.1.5
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command B in eng_detect_blink.log
> Followed by Command G in eng_detect_blink.log
> */
>
> -static int dediprog_command_j(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x07, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command J failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
> -#endif
>
> static int parse_voltage(char *voltage)
>  {
>    char *tmp = NULL;
> @@ -584,22 +556,35 @@
>    	return 1;
>    }
>    dediprog_endpoint = 2;
> dediprog_set_leds(PASS_ON|BUSY_ON|ERROR_ON);
> /* URB 6. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 7. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 8. Command Prepare Receive Device String. */
> /* URB 9. Command Receive Device String. */
> if (dediprog_check_devicestring())
> if (dediprog_check_devicestring()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 10. Command C. */
> if (dediprog_command_c())
> if (dediprog_command_c()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 11. Command Set SPI Voltage. */
> if (dediprog_set_spi_voltage(millivolt))
>
> if (dediprog_set_spi_voltage(millivolt)) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return 1;
>
> }
>
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_DEDIPROG;
>
> @@ -610,6 +595,8 @@
>    dediprog_do_stuff();
>  #endif
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> @@ -670,10 +657,6 @@
>  {
>    msg_pspew("%s\n", __func__);
> /* Shutdown on firmware 5.x */
> if (dediprog_firmwareversion == 5)
> if (dediprog_command_i())
> 	return 1;
> /* URB 28. Command Set SPI Voltage to 0. */
> if (dediprog_set_spi_voltage(0x0))
> return 1;

Otherwise looks good to me.

Tested-by: Mathias Krause

With the above mentioned changes:

Acked-by: Mathias Krause