Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

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 <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> Tested-by: Guenter Roeck <linux@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(),
>>   };