Re: [PATCH 08/10] esp: convert ESPState pdma_cb from a function pointer to an integer

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

On 28/2/22 23:25, Mark Cave-Ayland wrote:
> This prepares for the inclusion of the current PDMA callback in the migration
> stream since the callback is referenced by an integer instead of a function
> pointer.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c         | 47 ++++++++++++++++++++++++++++++-------------
>   include/hw/scsi/esp.h | 11 +++++++++-
>   2 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 035fb0d1f6..e8b6f25dbb 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -195,14 +195,10 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
>       esp_set_tc(s, dmalen);
>   }
>   
> -static void esp_set_pdma_cb(ESPState *s, void (*cb)(ESPState *))
> +static void esp_set_pdma_cb(ESPState *s, int pdma_cb)

Why signed?

>   {
> -    s->pdma_cb = cb;
> -}
> -
> -static void esp_pdma_cb(ESPState *s)
> -{
> -    s->pdma_cb(s);
> +    assert(pdma_cb >= 0 && pdma_cb < PDMA_NUM_CBS);

No need to check >=0 if using unsigned.

> +    s->pdma_cb = pdma_cb;
>   }

> +static void esp_pdma_cb(ESPState *s)
> +{
> +    switch (s->pdma_cb) {
> +    case PDMA_SATN_PDMA_CB:
> +        satn_pdma_cb(s);
> +        break;
> +    case PDMA_S_WITHOUT_SATN_PDMA_CB:
> +        s_without_satn_pdma_cb(s);
> +        break;
> +    case PDMA_SATN_STOP_PDMA_CB:
> +        satn_stop_pdma_cb(s);
> +        break;
> +    case PDMA_WRITE_RESPONSE_PDMA_CB:
> +        write_response_pdma_cb(s);
> +        break;
> +    case PDMA_DO_DMA_PDMA_CB:
> +        do_dma_pdma_cb(s);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +

> index b1ec27612f..885ccf4660 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -51,7 +51,7 @@ struct ESPState {
>       ESPDMAMemoryReadWriteFunc dma_memory_write;
>       void *dma_opaque;
>       void (*dma_cb)(ESPState *s);
> -    void (*pdma_cb)(ESPState *s);
> +    uint8_t pdma_cb;

And here you use unsigned :)

>       uint8_t mig_version_id;
>   
> @@ -150,6 +150,15 @@ struct SysBusESPState {
>   #define TCHI_FAS100A 0x4
>   #define TCHI_AM53C974 0x12
>   
> +/* PDMA callbacks */
> +#define PDMA_SATN_PDMA_CB              0
> +#define PDMA_S_WITHOUT_SATN_PDMA_CB    1
> +#define PDMA_SATN_STOP_PDMA_CB         2
> +#define PDMA_WRITE_RESPONSE_PDMA_CB    3
> +#define PDMA_DO_DMA_PDMA_CB            4

I'd rather use an enum (and in esp_pdma_cb's switch).

> +#define PDMA_NUM_CBS                   5
> +
>   void esp_dma_enable(ESPState *s, int irq, int level);
>   void esp_request_cancelled(SCSIRequest *req);
>   void esp_command_complete(SCSIRequest *req, size_t resid);

Preferrably using unsigned:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>