Re: [Qemu-devel] [PATCH v1 1/3] i.MX: Add GPIO device

[Peter Crosthwaite]

On Sun, Sep 6, 2015 at 2:19 PM, Jean-Christophe DUBOIS
<address@hidden> wrote:
> Le 06/09/2015 20:23, Peter Crosthwaite a écrit :
>>
>> On Sun, Sep 6, 2015 at 5:55 AM, Jean-Christophe DUBOIS
>> <address@hidden> wrote:
>>>
>>> Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
>>>>
>>>> On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
>>>> <address@hidden> wrote:
>>>>>
>>>>> Signed-off-by: Jean-Christophe Dubois <address@hidden>
>>>>> ---
>>>>>    hw/gpio/Makefile.objs      |   1 +
>>>>>    hw/gpio/imx_gpio.c         | 358
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>    include/hw/gpio/imx_gpio.h |  60 ++++++++
>>>>>    3 files changed, 419 insertions(+)
>>>>>    create mode 100644 hw/gpio/imx_gpio.c
>>>>>    create mode 100644 include/hw/gpio/imx_gpio.h
>>>>>
>>>>> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
>>>>> index 1abcf17..52233f7 100644
>>>>> --- a/hw/gpio/Makefile.objs
>>>>> +++ b/hw/gpio/Makefile.objs
>>>>> @@ -5,3 +5,4 @@ common-obj-$(CONFIG_ZAURUS) += zaurus.o
>>>>>    common-obj-$(CONFIG_E500) += mpc8xxx.o
>>>>>
>>>>>    obj-$(CONFIG_OMAP) += omap_gpio.o
>>>>> +obj-$(CONFIG_IMX) += imx_gpio.o
>>>>> diff --git a/hw/gpio/imx_gpio.c b/hw/gpio/imx_gpio.c
>>>>> new file mode 100644
>>>>> index 0000000..8ec1d4c
>>>>> --- /dev/null
>>>>> +++ b/hw/gpio/imx_gpio.c
>>>>> @@ -0,0 +1,358 @@
>>>>> +/*
>>>>> + * i.MX processors GPIO emulation.
>>>>> + *
>>>>> + * Copyright (C) 2015 Jean-Christophe Dubois <address@hidden>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU General Public License as
>>>>> + * published by the Free Software Foundation; either version 2 or
>>>>> + * (at your option) version 3 of the License.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License
>>>>> along
>>>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>> + */
>>>>> +
>>>>> +#include "hw/gpio/imx_gpio.h"
>>>>> +
>>>>> +#ifndef IMX_GPIO_DEBUG
>>>>> +#define IMX_GPIO_DEBUG                 0
>>>>> +#endif
>>>>> +
>>>>> +#if IMX_GPIO_DEBUG
>>>>> +#  define DPRINTF(fmt, args...) \
>>>>> +          do { fprintf(stder, "%s: " fmt , __func__, ##args); } while
>>>>> (0)
>>>>> +
>>>>
>>>> Use a regular if for debug conditional. This is so the debug code is
>>>> always compiled.
>>>
>>> Not sure what you mean here ... Most files in qemu have DEBUG functions
>>> compiled out in the normal case ...
>>>
>> We are trying to slowly change that.
>>
>>> Is there a change in "politic" on this topic?
>>>
>> No this has been the policy for a while now.
>>
>>> Could you point to an implementation doing things right?
>>>
>> hw/dma/pl330.c
>
>
> Not sure, I understand. It is still compiled out as PL330_ERR_DEBUG is 0 in
> the "normal" case.
>

It is not compiled out textually, it is "if (0) {}"''d.

#ifndef PL330_ERR_DEBUG
#define PL330_ERR_DEBUG 0
#endif

#define DB_PRINT_L(lvl, fmt, args...) do {\
    if (PL330_ERR_DEBUG >= lvl) {\
        fprintf(stderr, "PL330: %s:" fmt, __func__, ## args);\
    } \
} while (0);

Note that #define DB_PRINT_L has no conditioning #ifdef which is what
we are going for. The idea is, even if the code is never run, it must
always still be compiled. This catches issues at compile time, when
major changes happen to core headers etc.

Regards,
Peter

> JC
>
>
>>
>> Regards,
>> Peter
>>
>>>>> +static char const *imx_gpio_reg_name(uint32_t reg)
>>>>> +{
>>>>> +    switch (reg) {
>>>>> +    case DR_ADDR:
>>>>> +        return "DR";
>>>>> +    case GDIR_ADDR:
>
>