Re: [PATCH 2/4] hw/lm32/milkymist: Comment to remember some IRQs lines are left unwired

[Philippe Mathieu-Daudé]

On 7/6/20 8:32 PM, Alistair Francis wrote:
> On Mon, Jul 6, 2020 at 11:04 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
>>
>> On 7/6/20 6:19 PM, Alistair Francis wrote:
>>> On Sun, Jul 5, 2020 at 2:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>> wrote:
>>>>
>>>> The 'card is readonly' and 'card inserted' IRQs are not wired.
>>>> Add a comment in case someone know where to wire them.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> I'm not convinced adding fixmes or todos in the code is the right
>>> direction. It would be better to file bugs or use some other more
>>> official tracking mechanism.
>>
>> This code is orphan :S
>>
>> I'll fill a launchpad bug ticket.
> 
> I also mean in general (you have some other patches that add TODOs or FIXMEs).

Not all developers look at launchpad, while all of them read the code ;)

$ git grep -E '(TODO|FIXME)' | wc -l
1899

For orphan code, a comment in the code might be better to remember
the technical debt to the next developers interested to rework this
piece of code (I'd rather not trust they'll dig in the mailing list
archive and launchpad tickets while staring at the code).

> 
>>
>> OTOH we could also log UNIMP for lost IRQs (triggered but
>> no handler registered).
> 
> That would also work.
> 
> Alistair
> 
>>
>>>
>>> Alistair
>>>
>>>> ---
>>>>  hw/lm32/milkymist.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c
>>>> index 469e3c4322..117973c967 100644
>>>> --- a/hw/lm32/milkymist.c
>>>> +++ b/hw/lm32/milkymist.c
>>>> @@ -87,6 +87,7 @@ static DeviceState *milkymist_memcard_create(hwaddr base)
>>>>      dev = qdev_new("milkymist-memcard");
>>>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>>>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>>> +    /* FIXME wire 'card is readonly' and 'card inserted' IRQs? */
>>>>
>>>>      return dev;
>>>>  }
>>>> --
>>>> 2.21.3
>>>>
>>>>
>>>
>