Re: [Qemu-devel] [PATCH v3 1/4] SSI: Built in multiple device support

[Peter Crosthwaite]

On Wed, Apr 25, 2012 at 2:41 AM, Peter Maydell <address@hidden> wrote:
> On 20 April 2012 03:12, Peter A. G. Crosthwaite
> <address@hidden> wrote:
>> Added support for multiple devices attached to a single SSI bus (Previously
>> SSI masters with multiple slaves were emulated as multiple point to point SSI
>> busses)
>
>>  static struct BusInfo ssi_bus_info = {
>>     .name = "SSI",
>>     .size = sizeof(SSIBus),
>> +    .props = (Property[]) {
>> +        DEFINE_PROP_INT32("ss", struct SSISlave, ss, 0),
>
> "ss" is a terrible name for a property. Can we have something
> a little less abbreviated, please?
>
>>  SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
>>  {
>> -    BusState *bus;
>> -    bus = qbus_create(&ssi_bus_info, parent, name);
>> -    return FROM_QBUS(SSIBus, bus);
>> +    SSIBus *bus;
>> +
>> +    bus = FROM_QBUS(SSIBus, qbus_create(&ssi_bus_info, parent, name));
>> +    vmstate_register(NULL, -1, &vmstate_ssi_bus, bus);
>> +    return  bus;
>
> Stray double space.
>

ack

>> +void ssi_select_slave(SSIBus *bus, int32_t ss)
>> +{
>> +    SSISlave *slave;
>> +    SSISlaveClass *ssc;
>> +
>> +    if (bus->ss == ss) {
>> +        return;
>> +    }
>> +
>> +    slave = get_current_slave(bus);
>> +    if (slave) {
>> +        ssc = SSI_SLAVE_GET_CLASS(slave);
>> +        if (ssc->set_cs) {
>> +            ssc->set_cs(slave, 0);
>> +        }
>> +    }
>> +    bus->ss = ss;
>
> Something wrong here. If bus->ss is a property you can't modify
> it randomly at runtime. (It won't get put back to the right
> value at reset, for instance.)
>

Hi Peter,

Thanks for your review.

I think there is confusion here in that I have named both the bus and
slave properties ss. Each slave has a index which ive called ss and
the bus has a property ss. The property defintion above applies the
slave device ss property which is constant. I think this will go away
when I clear up the name confusion as you have suggested.

Regards,
Peter

> -- PMM