Re: [PATCH v2 01/42] esp: checkpatch fixes

[Mark Cave-Ayland]

On 01/03/2021 19:43, Laurent Vivier wrote:

> Le 09/02/2021 à 20:29, Mark Cave-Ayland a écrit :
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >   hw/scsi/esp.c | 52 ++++++++++++++++++++++++++++++---------------------
> >   1 file changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index b84e0fe33e..7073166ad1 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -241,8 +241,9 @@ static void handle_satn(ESPState *s)
> >       }
> >       s->pdma_cb = satn_pdma_cb;
> >       len = get_cmd(s, buf, sizeof(buf));
> > -    if (len)
> > +    if (len) {
> >           do_cmd(s, buf);
> > +    }
> >   }
> >   
> >   static void s_without_satn_pdma_cb(ESPState *s)
> > @@ -398,8 +399,8 @@ static void esp_do_dma(ESPState *s)
> >            * handle_ti_cmd() with do_cmd != NULL (see the assert())
> >            */
> >           trace_esp_do_dma(s->cmdlen, len);
> > -        assert (s->cmdlen <= sizeof(s->cmdbuf) &&
> > -                len <= sizeof(s->cmdbuf) - s->cmdlen);
> > +        assert(s->cmdlen <= sizeof(s->cmdbuf) &&
> > +               len <= sizeof(s->cmdbuf) - s->cmdlen);
> >           if (s->dma_memory_read) {
> >               s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
> >           } else {
> > @@ -445,15 +446,18 @@ static void esp_do_dma(ESPState *s)
> >       s->dma_left -= len;
> >       s->async_buf += len;
> >       s->async_len -= len;
> > -    if (to_device)
> > +    if (to_device) {
> >           s->ti_size += len;
> > -    else
> > +    } else {
> >           s->ti_size -= len;
> > +    }
> >       if (s->async_len == 0) {
> >           scsi_req_continue(s->current_req);
> > -        /* If there is still data to be read from the device then
> > -           complete the DMA operation immediately.  Otherwise defer
> > -           until the scsi layer has completed.  */
> > +        /*
> > +         * If there is still data to be read from the device then
> > +         * complete the DMA operation immediately.  Otherwise defer
> > +         * until the scsi layer has completed.
> > +         */
> >           if (to_device || s->dma_left != 0 || s->ti_size == 0) {
> >               return;
> >           }
> > @@ -491,7 +495,8 @@ void esp_command_complete(SCSIRequest *req, uint32_t status,
> >       ESPState *s = req->hba_private;
> >   
> >       if (s->rregs[ESP_RSTAT] & STAT_INT) {
> > -        /* Defer handling command complete until the previous
> > +        /*
> > +         * Defer handling command complete until the previous
> >            * interrupt has been handled.
> >            */
> >           trace_esp_command_complete_deferred();
> > @@ -513,8 +518,10 @@ void esp_transfer_data(SCSIRequest *req, uint32_t len)
> >       if (s->dma_left) {
> >           esp_do_dma(s);
> >       } else if (s->dma_counter != 0 && s->ti_size <= 0) {
> > -        /* If this was the last part of a DMA transfer then the
> > -           completion interrupt is deferred to here.  */
> > +        /*
> > +         * If this was the last part of a DMA transfer then the
> > +         * completion interrupt is deferred to here.
> > +         */
> >           esp_dma_done(s);
> >       }
> >   }
> > @@ -531,17 +538,18 @@ static void handle_ti(ESPState *s)
> >       dmalen = s->rregs[ESP_TCLO];
> >       dmalen |= s->rregs[ESP_TCMID] << 8;
> >       dmalen |= s->rregs[ESP_TCHI] << 16;
> > -    if (dmalen==0) {
> > -      dmalen=0x10000;
> > +    if (dmalen == 0) {
> > +        dmalen = 0x10000;
> >       }
> >       s->dma_counter = dmalen;
> >   
> > -    if (s->do_cmd)
> > +    if (s->do_cmd) {
> >           minlen = (dmalen < ESP_CMDBUF_SZ) ? dmalen : ESP_CMDBUF_SZ;
> > -    else if (s->ti_size < 0)
> > +    } else if (s->ti_size < 0) {
> >           minlen = (dmalen < -s->ti_size) ? dmalen : -s->ti_size;
> > -    else
> > +    } else {
> >           minlen = (dmalen < s->ti_size) ? dmalen : s->ti_size;
> > +    }
> >       trace_esp_handle_ti(minlen);
> >       if (s->dma) {
> >           s->dma_left = minlen;
> > @@ -606,8 +614,10 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
> >           }
> >           break;
> >       case ESP_RINTR:
> > -        /* Clear sequence step, interrupt register and all status bits
> > -           except TC */
> > +        /*
> > +         * Clear sequence step, interrupt register and all status bits
> > +         * except TC
> > +         */
> >           old_val = s->rregs[ESP_RINTR];
> >           s->rregs[ESP_RINTR] = 0;
> >           s->rregs[ESP_RSTAT] &= ~STAT_TC;
> > @@ -665,13 +675,13 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t 
> > val)
> >           } else {
> >               s->dma = 0;
> >           }
> > -        switch(val & CMD_CMD) {
> > +        switch (val & CMD_CMD) {
> >           case CMD_NOP:
> >               trace_esp_mem_writeb_cmd_nop(val);
> >               break;
> >           case CMD_FLUSH:
> >               trace_esp_mem_writeb_cmd_flush(val);
> > -            //s->ti_size = 0;
> > +            /*s->ti_size = 0;*/
>
> Perhaps the line can simply be removed?

Phil pointed this out in one of his reviews, so I've removed it in my latest branch 
but in a later patch (I think after flush is implemented at which point we know it 
really isn't needed?).

> >               s->rregs[ESP_RINTR] = INTR_FC;
> >               s->rregs[ESP_RSEQ] = 0;
> >               s->rregs[ESP_RFLAGS] = 0;
> > @@ -787,7 +797,7 @@ static const VMStateDescription vmstate_esp_pdma = {
> >   };
> >   
> >   const VMStateDescription vmstate_esp = {
> > -    .name ="esp",
> > +    .name = "esp",
> >       .version_id = 4,
> >       .minimum_version_id = 3,
> >       .fields = (VMStateField[]) {
>
> Anyway:
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>


ATB,

Mark.