Re: [PATCH 1/3] hw/qdev: Restrict qdev_get_gpio_out_connector() to qdev-internal.h

[Philippe Mathieu-Daudé]

On 12/31/21 08:30, wangyanan (Y) wrote:
> Hi,
> 
> On 2021/12/30 6:52, Philippe Mathieu-Daudé wrote:
>> qdev_get_gpio_out_connector() is called by sysbus_get_connected_irq()
>> which is only used by platform-bus.c; restrict it to hw/core/ by
>> adding a local "qdev-internal.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/core/qdev-internal.h | 15 +++++++++++++++
>>   include/hw/qdev-core.h  | 18 ------------------
>>   hw/core/gpio.c          |  1 +
>>   hw/core/sysbus.c        |  1 +
>>   4 files changed, 17 insertions(+), 18 deletions(-)
>>   create mode 100644 hw/core/qdev-internal.h
>>
>> diff --git a/hw/core/qdev-internal.h b/hw/core/qdev-internal.h
>> new file mode 100644
>> index 00000000000..6ec17d0ea70
>> --- /dev/null
>> +++ b/hw/core/qdev-internal.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * qdev internal helpers
>> + *
>> + * Copyright (c) 2009-2021 QEMU contributors
>> + */
>> +#ifndef HW_CORE_QDEV_INTERNAL_H
>> +#define HW_CORE_QDEV_INTERNAL_H
>> +
>> +#include "hw/qdev-core.h"
>> +
>> +/* Following functions are only used by the platform-bus subsystem */
> Could it be better to also keep the original function comment here?

We could, but this include being now internal, it seems superfluous.

Since Peter documented this function, let see if he has an preference.

>> +qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>> *name, int n);
>> +
>> +#endif /* HW_CORE_QDEV_INTERNAL_H */
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index d19c9417520..655899654bb 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -532,24 +532,6 @@ void qdev_connect_gpio_out(DeviceState *dev, int
>> n, qemu_irq pin);
>>   void qdev_connect_gpio_out_named(DeviceState *dev, const char *name,
>> int n,
>>                                    qemu_irq input_pin);
>>   -/**
>> - * qdev_get_gpio_out_connector: Get the qemu_irq connected to an
>> output GPIO
>> - * @dev: Device whose output GPIO we are interested in
>> - * @name: Name of the output GPIO array
>> - * @n: Number of the output GPIO line within that array
>> - *
>> - * Returns whatever qemu_irq is currently connected to the specified
>> - * output GPIO line of @dev. This will be NULL if the output GPIO line
>> - * has never been wired up to the anything.  Note that the qemu_irq
>> - * returned does not belong to @dev -- it will be the input GPIO or
>> - * IRQ of whichever device the board code has connected up to @dev's
>> - * output GPIO.
>> - *
>> - * You probably don't need to use this function -- it is used only
>> - * by the platform-bus subsystem.
>> - */
>> -qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char
>> *name, int n);
>> -
>>   /**
>>    * qdev_intercept_gpio_out: Intercept an existing GPIO connection
>>    * @dev: Device to intercept the outbound GPIO line from
>> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
>> index 80d07a6ec99..513ccbd1062 100644
>> --- a/hw/core/gpio.c
>> +++ b/hw/core/gpio.c
>> @@ -21,6 +21,7 @@
>>   #include "hw/qdev-core.h"
>>   #include "hw/irq.h"
>>   #include "qapi/error.h"
>> +#include "qdev-internal.h"
>>     static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev,
>>                                                  const char *name)
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 05c1da3d311..0e6773c8df7 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/sysbus.h"
>>   #include "monitor/monitor.h"
>>   #include "exec/address-spaces.h"
>> +#include "qdev-internal.h"
>>     static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int
>> indent);
>>   static char *sysbus_get_fw_dev_path(DeviceState *dev);
> Otherwise, the tweak looks reasonable:
> Reviewed-by: Yanan Wang <wangyanan55@huawei.com>

Thanks,

Phil.