On 12/28/20 8:30 PM, Mark Cave-Ayland wrote: > On 27/12/2020 22:13, BALATON Zoltan wrote: > >> We'll need a flag for implementing some device specific behaviour in >> via-ide but we already have a currently CMD646 specific field that can >> be repurposed for this and leave room for further flags if needed in >> the future. This patch changes the "secondary" field to "flags" and >> change CMD646 and its users accordingly and define a new flag for >> forcing legacy mode that will be used by via-ide for now. >> >> Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> >> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> Reviewed-by: Guenter Roeck <li...@roeck-us.net> >> Tested-by: Guenter Roeck <li...@roeck-us.net> >> --- >> v2: Fixed typo in commit message >> >> hw/ide/cmd646.c | 4 ++-- >> hw/sparc64/sun4u.c | 2 +- >> include/hw/ide/pci.h | 7 ++++++- >> 3 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >> index c254631485..7a96016116 100644 >> --- a/hw/ide/cmd646.c >> +++ b/hw/ide/cmd646.c >> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, >> Error **errp) >> pci_conf[PCI_CLASS_PROG] = 0x8f; >> pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0 > > Doesn't the existing comment above cause checkpatch to fail?
The comment is old and Balaton didn't modified it. Usually I'd prefer to address style change in a separate commit, ... > >> - if (d->secondary) { >> + if (d->flags & BIT(PCI_IDE_SECONDARY)) { >> /* XXX: if not enabled, really disable the seconday IDE >> controller */ >> pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */ ... but since a similar comment is added here, it might be acceptable to fix the style of the former one here too. I noted Balaton already addressed your comment in a v3. >> } >> @@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev) >> } >> static Property cmd646_ide_properties[] = { >> - DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0), >> + DEFINE_PROP_BIT("secondary", PCIIDEState, flags, >> PCI_IDE_SECONDARY, false), >> DEFINE_PROP_END_OF_LIST(), >> };