Re: [Qemu-block] [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one

[Alistair Francis]

On Thu, Dec 14, 2017 at 10:40 AM, Philippe Mathieu-Daudé
<address@hidden> wrote:
>>>  /* Capabilities registers provide information on supported features of this
>>>   * specific host controller implementation */
>>> -static Property sdhci_pci_properties[] = {
>>> +static Property sdhci_properties[] = {
>>>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>>              SDHC_CAPAB_REG_DEFAULT),
>>>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> +    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, 
>>> pending_insert_quirk,
>>> +                     false),
>>
>> I like the reduction of code in this patch, but aren't we now going to
>> have device properties that aren't actually connected to anything?
>
> I'm not sure I understand, ar you worried about the PCI_SDHCI will now
> have this property but not use it?
>
> I couldn't find any machine using SDHCI via PCI and was tempted to
> just remove this code,
> the only references to it is the REDHAT_SDHCI from commits ece5e5bfa13
> and 224d10ff5ae.

So it is also possible to set these properties from the command line.

If I'm a user and I see that my device has a property "foo" I expect
that setting that property will have an affect on that device. So
there shouldn't be properties that aren't actually connected to the
device.

Alistair

>
> Maybe an attempt to write SDHCI qtests via PCI?
>
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> +/* --- qdev PCI --- */
>>> +
>>>  static void sdhci_pci_realize(PCIDevice *dev, Error **errp)
>>>  {
>>>      SDHCIState *s = PCI_SDHCI(dev);
>>> @@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, 
>>> void *data)
>>>      k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>>      dc->vmsd = &sdhci_vmstate;
>>> -    dc->props = sdhci_pci_properties;
>>> +    dc->props = sdhci_properties;
>>>      dc->reset = sdhci_poweron_reset;
>>>  }
>>>
>>> @@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info = {
>>>      },
>>>  };
>>>
>>> -static Property sdhci_sysbus_properties[] = {
>>> -    DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>>> -            SDHC_CAPAB_REG_DEFAULT),
>>> -    DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
>>> -    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, 
>>> pending_insert_quirk,
>>> -                     false),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> +/* --- qdev SysBus --- */
>