Re: [Qemu-trivial] [PATCH 10/11] arm: add missing static and remove unused functions

[Blue Swirl]

On Sun, Oct 14, 2012 at 8:09 PM, Peter Maydell <address@hidden> wrote:
> On 14 October 2012 20:58, Blue Swirl <address@hidden> wrote:
>> index 2fc4137..2c02a83 100644
>> --- a/hw/omap_gpmc.c
>> +++ b/hw/omap_gpmc.c
>> @@ -871,24 +871,3 @@ void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, 
>> MemoryRegion *iomem)
>>      f->iomem = iomem;
>>      omap_gpmc_cs_map(s, cs);
>>  }
>> -
>> -void omap_gpmc_attach_nand(struct omap_gpmc_s *s, int cs, DeviceState *nand)
>> -{
>> -    struct omap_gpmc_cs_file_s *f;
>> -    assert(nand);
>> -
>> -    if (cs < 0 || cs >= 8) {
>> -        fprintf(stderr, "%s: bad chip-select %i\n", __func__, cs);
>> -        exit(-1);
>> -    }
>> -    f = &s->cs_file[cs];
>> -
>> -    omap_gpmc_cs_unmap(s, cs);
>> -    f->config[0] &= ~(0xf << 10);
>> -    f->config[0] |= (OMAP_GPMC_NAND << 10);
>> -    f->dev = nand;
>> -    if (nand_getbuswidth(f->dev) == 16) {
>> -        f->config[0] |= OMAP_GPMC_16BIT << 12;
>> -    }
>> -    omap_gpmc_cs_map(s, cs);
>> -}
>
> Please don't delete this function, it is the public facing interface
> for allowing board models to attach NAND devices to the GPMC. This
> might not be used by anything currently in mainline, but it is used
> by the omap3 beagle and overo board models in qemu-linaro (and which
> I will upstream eventually, honest).

It could be re-added with the boards, or just disabled now with #if 0/#endif.

> More generally I'm wary of deletion of apparently unused functions like
> this one (or some of the others like the 'pcmcia eject card' function)
> which are obviously intended to be public facing APIs to other
> device models, unless they come with rationales along the lines of
> "this function was added for purpose X but it is not needed now because
> commit Y changed things so you do this with Z now".

In case of pcmcia, it's from 2009 so one would expect users to have
appeared already.

>
>> diff --git a/linux-user/arm/nwfpe/fpa11.c b/linux-user/arm/nwfpe/fpa11.c
>> index eebd93f..3434036 100644
>> --- a/linux-user/arm/nwfpe/fpa11.c
>> +++ b/linux-user/arm/nwfpe/fpa11.c
>
> In general anything in linux-user/arm/nwfpe is legacy code which
> it's scarcely worth the effort of touching or reviewing.

Adding 'static' should be only beneficial.

>
> thanks
> -- PMM