From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57428) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ePZJh-0006ML-O8 for qemu-devel@nongnu.org; Thu, 14 Dec 2017 14:37:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ePZJg-0008Pq-RN for qemu-devel@nongnu.org; Thu, 14 Dec 2017 14:37:13 -0500 MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: References: <20171213195852.30439-1-f4bug@amsat.org> <20171213195852.30439-7-f4bug@amsat.org> From: Alistair Francis Date: Thu, 14 Dec 2017 11:36:38 -0800 Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Cc: Alistair Francis , Kevin O'Connor , Paolo Bonzini , Gerd Hoffmann , "Edgar E . Iglesias" , Peter Maydell , Prasad J Pandit , "open list:Block layer core" , Peter Crosthwaite , Andrey Smirnov , "qemu-devel@nongnu.org Developers" , Andrew Baumann , Sai Pavan Boddu , Andrey Yurovsky On Thu, Dec 14, 2017 at 10:40 AM, Philippe Mathieu-Daud=C3=A9 wrote: >>> /* Capabilities registers provide information on supported features of= this >>> * specific host controller implementation */ >>> -static Property sdhci_pci_properties[] =3D { >>> +static Property sdhci_properties[] =3D { >>> 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_inser= t_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 =3D PCI_SDHCI(dev); >>> @@ -1295,7 +1299,7 @@ static void sdhci_pci_class_init(ObjectClass *kla= ss, void *data) >>> k->class_id =3D PCI_CLASS_SYSTEM_SDHCI; >>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >>> dc->vmsd =3D &sdhci_vmstate; >>> - dc->props =3D sdhci_pci_properties; >>> + dc->props =3D sdhci_properties; >>> dc->reset =3D sdhci_poweron_reset; >>> } >>> >>> @@ -1310,14 +1314,7 @@ static const TypeInfo sdhci_pci_info =3D { >>> }, >>> }; >>> >>> -static Property sdhci_sysbus_properties[] =3D { >>> - 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_inser= t_quirk, >>> - false), >>> - DEFINE_PROP_END_OF_LIST(), >>> -}; >>> +/* --- qdev SysBus --- */ >