Re: [Qemu-devel] [PATCH 09/13] hw/char/exynos4210_uart.c: Remove unneeded handling of NULL chardev

[Philippe Mathieu-Daudé]

Hi Thomas,

On 04/25/2018 12:05 PM, Thomas Huth wrote:
> On 20.04.2018 16:52, Peter Maydell wrote:
>> The handling of NULL chardevs in exynos4210_uart_create() is now
>> all unnecessary: we don't need to create 'null' chardevs, and we
>> don't need to enforce a bounds check on serial_hd().
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/char/exynos4210_uart.c | 20 --------------------
>>  1 file changed, 20 deletions(-)
>>
>> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>> index c2bba03362..a5a285655f 100644
>> --- a/hw/char/exynos4210_uart.c
>> +++ b/hw/char/exynos4210_uart.c
>> @@ -589,28 +589,8 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
>>      DeviceState  *dev;
>>      SysBusDevice *bus;
>>  
>> -    const char chr_name[] = "serial";
>> -    char label[ARRAY_SIZE(chr_name) + 1];
>> -
>>      dev = qdev_create(NULL, TYPE_EXYNOS4210_UART);
>>  
>> -    if (!chr) {
>> -        if (channel >= MAX_SERIAL_PORTS) {
>> -            error_report("Only %d serial ports are supported by QEMU",
>> -                         MAX_SERIAL_PORTS);
>> -            exit(1);
>> -        }
> 
> If I get the EXYNOS 4210 data sheet right, this chip has only 4 channels
> indeed:
> 
> http://www.samsung.com/global/business/semiconductor/file/product/Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf
> 
> So I think you should at least keep an "assert(channel < 4)" in here?

This file models a single UART, I don't think a such assert belongs here.

Although it is named EXYNOS4210, I'm pretty sure it should works to
model UARTs from other SoCs from the Exynos4xxx series.

There are no restriction on newer SoCs to have > 4 UARTs.

For this particular Exynos4210 SoC, the 4 channels are correctly limited
in exynos4210_init().

Regards,

Phil.

> 
> Apart from that, the patch looks sane to me, so when you add that assert():
> 
> Reviewed-by: Thomas Huth <address@hidden>