Re: [PATCH v4 07/45] Implement BCM2838 GPIO functionality

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:33, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/gpio/bcm2838_gpio.c | 192 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 189 insertions(+), 3 deletions(-)

>  static uint64_t bcm2838_gpio_read(void *opaque, hwaddr offset, unsigned size)
>  {
> +    BCM2838GpioState *s = (BCM2838GpioState *)opaque;
>      uint64_t value = 0;
>
> -    qemu_log_mask(LOG_UNIMP, "%s: %s: not implemented for %"HWADDR_PRIx"\n",
> -                  TYPE_BCM2838_GPIO, __func__, offset);
> +    switch (offset) {
> +    case GPFSEL0:
> +    case GPFSEL1:
> +    case GPFSEL2:
> +    case GPFSEL3:
> +    case GPFSEL4:
> +    case GPFSEL5:
> +        value = gpfsel_get(s, offset / BYTES_IN_WORD);
> +        break;
> +    case GPSET0:
> +    case GPSET1:
> +    case GPCLR0:
> +    case GPCLR1:
> +        /* Write Only */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: %s: Attempt reading from write 
> only"
> +                      " register. %lu will be returned. Address 
> 0x%"HWADDR_PRIx
> +                      ", size %u\n", TYPE_BCM2838_GPIO, __func__, value, 
> offset,
> +                      size);

'value' is a uint64_t, but you try to print it with a %lu format
string here. This won't compile on 32-bit machines. (In general
watch out for %lu %lx etc and don't use them unless the type
really is "long".)

You can get the compiler to tell you about these format issues by
any of the following options:
 * building for a 32-bit host (eg i386)
 * building for macos (the macos clang is stricter about these even
   when building for a 64-bit)
 * running your patches through the gitlab CI setup: fork the QEMU
   project on gitlab as an ordinary gitlab user; then push your
   branch to your fork of the repo with some environment variables
   set to trigger a CI pipeline run:
   https://www.qemu.org/docs/master/devel/ci.html#custom-ci-cd-variables


>  static void bcm2838_gpio_reset(DeviceState *dev)
>  {
>      BCM2838GpioState *s = BCM2838_GPIO(dev);
>
> +    memset(s->fsel, 0, sizeof(s->fsel));
> +
>      s->lev0 = 0;
>      s->lev1 = 0;

I think this bit should go in the previous patch since we added
s->fsel there and do the other reset code there already.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM