On 9/24/21 09:16, Mark Cave-Ayland wrote:
On 23/09/2021 15:16, BALATON Zoltan wrote:

On Thu, 23 Sep 2021, Mark Cave-Ayland wrote:
Convert nubus_device_realize() to use a bitmap to manage available slots to allow for future Nubus devices to be plugged into arbitrary slots from the command line
using a new qdev "slot" parameter for nubus devices.

Update mac_nubus_bridge_init() to only allow slots 0x9 to 0xe on a Macintosh machines as documented in "Desigining Cards and Drivers for the Macintosh Family".

Small typo: "a Macintosh machnies", either a or s is not needed.

Thanks - I've updated this for v6.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
hw/nubus/mac-nubus-bridge.c         |  4 ++++
hw/nubus/nubus-bus.c                |  5 +++--
hw/nubus/nubus-device.c             | 32 +++++++++++++++++++++++------
include/hw/nubus/mac-nubus-bridge.h |  4 ++++
include/hw/nubus/nubus.h            | 13 ++++++------
5 files changed, 43 insertions(+), 15 deletions(-)

struct NubusDevice {
    DeviceState qdev;

-    int slot;
+    int32_t slot;

Why uint32_t? Considering its max value even uint8_t would be enough although maybe invalid value would be 255 instead of -1 then. As this was added in previous patch you could avoid churn here by introducing it with the right type in that patch already. (But feel free to ignore it if you have no time for more changes, the current version works so if you don't do another version for other reasons this probably don't worth the effort alone.)

I think it makes sense to keep this signed since -1 is used for other bus implementations to indicate that an explicit slot hasn't been assigned. Potentially the slot number could be represented by an 8-bit value, however it seems there is no DEFINE_PROP_INT8 or DEFINE_PROP_INT16. Fortunately the slot number is restricted by the available slots bitmask anyhow, so this shouldn't be an issue.

I wondered the same and noticed there is no DEFINE_PROP_INT8, so didn't
want to bother Mark furthermore :) Adding & using DEFINE_PROP_INT8 seems
a good idea, but to be fair with the repository we'd need to audit the
other places where DEFINE_PROP_INT32 isn't justified and update. Extra
work for not much gain, so I'm find with this patch. Can be improved on
top.

Reply via email to