| Message ID | 20210209193018.31339-11-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | esp: consolidate PDMA transfer buffers and other fixes | expand |
On 2/9/21 8:29 PM, Mark Cave-Ayland wrote: > This simplifies reading the STC register value without having to manually shift > each individual 8-bit value. If possible repeat the subject so the sentence is easier to understand. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 10/02/2021 22:33, Philippe Mathieu-Daudé wrote: > On 2/9/21 8:29 PM, Mark Cave-Ayland wrote: >> This simplifies reading the STC register value without having to manually shift >> each individual 8-bit value. > > If possible repeat the subject so the sentence is easier to understand. I've always read commit messages as summary followed detail, so I've tended to avoid repetition if the context is obvious from the summary (a quick glance through my inbox suggest that quite a few authors also do the same). Perhaps adding in the word "function" would help readability here, e.g. "This function simplifies reading the STC register value..."? >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/scsi/esp.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ATB, Mark.
On 2/11/21 8:53 AM, Mark Cave-Ayland wrote: > On 10/02/2021 22:33, Philippe Mathieu-Daudé wrote: > >> On 2/9/21 8:29 PM, Mark Cave-Ayland wrote: >>> This simplifies reading the STC register value without having to >>> manually shift >>> each individual 8-bit value. >> >> If possible repeat the subject so the sentence is easier to understand. > > I've always read commit messages as summary followed detail, so I've > tended to avoid repetition if the context is obvious from the summary (a > quick glance through my inbox suggest that quite a few authors also do > the same). Not because other do it means it is a good practice ;) I suppose it depends on personal review workflow and email client used or gitk layout. In the one I use the commit description is displayed first, so I have to look elsewhere to prepend the patch subject which is using another font. I'm using Thunderbird on Fedora and X1 carbon but had to do some config change in the default config because the font was too small, unreadable, then I added some 1.5x zoom factor. It always looked ugly. Apparently checking that again it seems the Fedora I used was not supporting these now displays well but now it should work OK, but I'm still using this old config. Well, my bad. Not in mood to reinstall. Forget my comment about having "atomic" commit descriptions then. > > Perhaps adding in the word "function" would help readability here, e.g. > "This function simplifies reading the STC register value..."? Sounds better to me :) > >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/scsi/esp.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > ATB, > > Mark. >
On 11/02/2021 10:07, Philippe Mathieu-Daudé wrote: > On 2/11/21 8:53 AM, Mark Cave-Ayland wrote: >> On 10/02/2021 22:33, Philippe Mathieu-Daudé wrote: >> >>> On 2/9/21 8:29 PM, Mark Cave-Ayland wrote: >>>> This simplifies reading the STC register value without having to >>>> manually shift >>>> each individual 8-bit value. >>> >>> If possible repeat the subject so the sentence is easier to understand. >> >> I've always read commit messages as summary followed detail, so I've >> tended to avoid repetition if the context is obvious from the summary (a >> quick glance through my inbox suggest that quite a few authors also do >> the same). > > Not because other do it means it is a good practice ;) > > I suppose it depends on personal review workflow and email client used > or gitk layout. In the one I use the commit description is displayed > first, so I have to look elsewhere to prepend the patch subject which > is using another font. > I'm using Thunderbird on Fedora and X1 carbon but had to do some config > change in the default config because the font was too small, unreadable, > then I added some 1.5x zoom factor. It always looked ugly. Apparently > checking that again it seems the Fedora I used was not supporting these > now displays well but now it should work OK, but I'm still using this > old config. Well, my bad. Not in mood to reinstall. Forget my comment > about having "atomic" commit descriptions then. > >> >> Perhaps adding in the word "function" would help readability here, e.g. >> "This function simplifies reading the STC register value..."? > > Sounds better to me :) Okay - I've made this change, along with a similar change to the patch before which had similar wording. >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> --- >>>> hw/scsi/esp.c | 15 ++++++++++++--- >>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ATB, Mark.
Le 09/02/2021 à 20:29, Mark Cave-Ayland a écrit : > This simplifies reading the STC register value without having to manually shift > each individual 8-bit value. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 3a39450930..a1acc2c9bd 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -116,6 +116,17 @@ static void esp_set_tc(ESPState *s, uint32_t dmalen) > s->rregs[ESP_TCHI] = dmalen >> 16; > } > > +static uint32_t esp_get_stc(ESPState *s) > +{ > + uint32_t dmalen; > + > + dmalen = s->wregs[ESP_TCLO]; > + dmalen |= s->wregs[ESP_TCMID] << 8; > + dmalen |= s->wregs[ESP_TCHI] << 16; > + > + return dmalen; > +} > + > static void set_pdma(ESPState *s, enum pdma_origin_id origin, > uint32_t index, uint32_t len) > { > @@ -688,9 +699,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val) > if (val & CMD_DMA) { > s->dma = 1; > /* Reload DMA counter. */ > - s->rregs[ESP_TCLO] = s->wregs[ESP_TCLO]; > - s->rregs[ESP_TCMID] = s->wregs[ESP_TCMID]; > - s->rregs[ESP_TCHI] = s->wregs[ESP_TCHI]; > + esp_set_tc(s, esp_get_stc(s)); > } else { > s->dma = 0; > } > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 3a39450930..a1acc2c9bd 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -116,6 +116,17 @@ static void esp_set_tc(ESPState *s, uint32_t dmalen) s->rregs[ESP_TCHI] = dmalen >> 16; } +static uint32_t esp_get_stc(ESPState *s) +{ + uint32_t dmalen; + + dmalen = s->wregs[ESP_TCLO]; + dmalen |= s->wregs[ESP_TCMID] << 8; + dmalen |= s->wregs[ESP_TCHI] << 16; + + return dmalen; +} + static void set_pdma(ESPState *s, enum pdma_origin_id origin, uint32_t index, uint32_t len) { @@ -688,9 +699,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val) if (val & CMD_DMA) { s->dma = 1; /* Reload DMA counter. */ - s->rregs[ESP_TCLO] = s->wregs[ESP_TCLO]; - s->rregs[ESP_TCMID] = s->wregs[ESP_TCMID]; - s->rregs[ESP_TCHI] = s->wregs[ESP_TCHI]; + esp_set_tc(s, esp_get_stc(s)); } else { s->dma = 0; }
This simplifies reading the STC register value without having to manually shift each individual 8-bit value. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)