On Fri, 24 Sep 2021, Philippe Mathieu-Daudé wrote:
> 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-ayland@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.

That's why I said UINT8 for prop and treat -1 as 0xff but I agree this is 
not a big deal so I've also said I'm OK with the current version. If it 
would be more effort than Mark is willing to put in this now I can 
understand that and not pushing it. It's not something that's wrong or 
worth holding the series back for just a possible minor improvement to 
avoid some code churn.

Regards,
BALATON Zoltan