[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action |
Date: |
Tue, 5 Mar 2019 00:22:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
On 3/5/19 12:14 AM, Philippe Mathieu-Daudé wrote:
> On 3/4/19 7:09 PM, Sven Schnelle wrote:
>> This makes the code easier to read - no functional change.
>>
>> Signed-off-by: Sven Schnelle <address@hidden>
>> ---
>> hw/scsi/lsi53c895a.c | 27 ++++++++++++++++-----------
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index fb9c6db4b2..d1eb2cf074 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -201,6 +201,13 @@ enum {
>> LSI_DMA_IN_PROGRESS, /* DMA operation is in progress */
>> };
>>
>> +enum {
> Since someone unfamiliar with the device could add:
>
> LSI_MSG_ACTION_DEBUG,
>
>> + LSI_MSG_ACTION_COMMAND,
>> + LSI_MSG_ACTION_DISCONNECT,
>> + LSI_MSG_ACTION_DOUT,
>> + LSI_MSG_ACTION_DIN,
>
> ... it is safer to assign values when enum are not expected to change:
>
> LSI_MSG_ACTION_COMMAND = 0,
> LSI_MSG_ACTION_DISCONNECT = 1,
> LSI_MSG_ACTION_DOUT = 2,
> LSI_MSG_ACTION_DIN = 3,
>
> With that change:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
>> +};
>> +
>> typedef struct {
>> /*< private >*/
>> PCIDevice parent_obj;
>> @@ -214,8 +221,6 @@ typedef struct {
>>
>> int carry; /* ??? Should this be an a visible register somewhere? */
>> int status;
>> - /* Action to take at the end of a MSG IN phase.
>> - 0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN. */
>> int msg_action;
I'd also declare 'enum msg_action' here (same comments as patch 2 of
this series).
Regardless the Reviewed-by tag stands.
>> int msg_len;
>> uint8_t msg[LSI_MAX_MSGIN_LEN];
>> @@ -323,7 +328,7 @@ static void lsi_soft_reset(LSIState *s)
>> trace_lsi_reset();
>> s->carry = 0;
>>
>> - s->msg_action = 0;
>> + s->msg_action = LSI_MSG_ACTION_COMMAND;
>> s->msg_len = 0;
>> s->waiting = LSI_NOWAIT;
>> s->dsa = 0;
>> @@ -686,7 +691,7 @@ static void lsi_reselect(LSIState *s, lsi_request *p)
>> trace_lsi_reselect(id);
>> s->scntl1 |= LSI_SCNTL1_CON;
>> lsi_set_phase(s, PHASE_MI);
>> - s->msg_action = p->out ? 2 : 3;
>> + s->msg_action = p->out ? LSI_MSG_ACTION_DOUT : LSI_MSG_ACTION_DIN;
>> s->current->dma_len = p->pending;
>> lsi_add_msg_byte(s, 0x80);
>> if (s->current->tag & LSI_TAG_VALID) {
>> @@ -857,7 +862,7 @@ static void lsi_do_command(LSIState *s)
>> lsi_add_msg_byte(s, 4); /* DISCONNECT */
>> /* wait data */
>> lsi_set_phase(s, PHASE_MI);
>> - s->msg_action = 1;
>> + s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>> lsi_queue_command(s);
>> } else {
>> /* wait command complete */
>> @@ -878,7 +883,7 @@ static void lsi_do_status(LSIState *s)
>> s->sfbr = status;
>> pci_dma_write(PCI_DEVICE(s), s->dnad, &status, 1);
>> lsi_set_phase(s, PHASE_MI);
>> - s->msg_action = 1;
>> + s->msg_action = LSI_MSG_ACTION_DISCONNECT;
>> lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */
>> }
>>
>> @@ -901,16 +906,16 @@ static void lsi_do_msgin(LSIState *s)
>> /* ??? Check if ATN (not yet implemented) is asserted and maybe
>> switch to PHASE_MO. */
>> switch (s->msg_action) {
>> - case 0:
>> + case LSI_MSG_ACTION_COMMAND:
>> lsi_set_phase(s, PHASE_CMD);
>> break;
>> - case 1:
>> + case LSI_MSG_ACTION_DISCONNECT:
>> lsi_disconnect(s);
>> break;
>> - case 2:
>> + case LSI_MSG_ACTION_DOUT:
>> lsi_set_phase(s, PHASE_DO);
>> break;
>> - case 3:
>> + case LSI_MSG_ACTION_DIN:
>> lsi_set_phase(s, PHASE_DI);
>> break;
>> default:
>> @@ -1062,7 +1067,7 @@ bad:
>> qemu_log_mask(LOG_UNIMP, "Unimplemented message 0x%02x\n", msg);
>> lsi_set_phase(s, PHASE_MI);
>> lsi_add_msg_byte(s, 7); /* MESSAGE REJECT */
>> - s->msg_action = 0;
>> + s->msg_action = LSI_MSG_ACTION_COMMAND;
>> }
>>
>> #define LSI_BUF_SIZE 4096
>>
- [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 3/5] lsi: use enum type for s->msg_action, Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 5/5] lsi: return dfifo value, Sven Schnelle, 2019/03/04
- [Qemu-devel] [PATCH 4/5] lsi: use SCSI phase names instead of numbers in trace, Sven Schnelle, 2019/03/04
- Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Eric Blake, 2019/03/04
- Re: [Qemu-devel] [PATCH 1/5] lsi: use ldn_le_p()/stn_le_p(), Philippe Mathieu-Daudé, 2019/03/04