On 9/11/24 1:22 AM, Damien Le Moal wrote: [...] >>>>>> Replace now gone out of fashion defined(CONFIG_PATA_TOSHIBA[_MODULE]) >> >> I'll probably rephrase this a bit in v2... >> [...] >> >>>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> >>>> >>>> [...[ >>>> >>>>>> Index: linux/drivers/ata/ata_generic.c >>>>>> =================================================================== >>>>>> --- linux.orig/drivers/ata/ata_generic.c >>>>>> +++ linux/drivers/ata/ata_generic.c >>>>>> @@ -220,7 +220,7 @@ static struct pci_device_id ata_generic[ >>>>>> { PCI_DEVICE(PCI_VENDOR_ID_OPTI, PCI_DEVICE_ID_OPTI_82C558), }, >>>>>> { PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE), >>>>>> .driver_data = ATA_GEN_FORCE_DMA }, >>>>>> -#if !defined(CONFIG_PATA_TOSHIBA) && !defined(CONFIG_PATA_TOSHIBA_MODULE) >>>>>> +#if !IS_ENABLED(CONFIG_PATA_TOSHIBA) >>>>> >>>>> I do not understand the negation here... It seems very wrong. If the driver is >>>>> indeed enabled, we need to add its PCI ID, no ? and the reverse when not defined... >>>> >>>> The separate driver was added by Alan Cox in 2009, before that >>>> Toshiba Piccolo controllers were handled by this generic driver... >>> >>> OK, makes sense now. Maybe we should add a comment above that IS_ENABLED() to >>> say so ? >> >> Makes sense, indeed. Do you think this is acceptable to be done in v2 of this >> patch? > > Yep, that is fine and would fit with the config option renaming. I started respinnig this patch and decided to add the Piccolo comment in a separate patch, while deferring the Kconfig entry rename until/iff it becomes truly necessary, as per Niklas' comment). Unfortunately, I seem to be late for the coming merge window... :-/ MBR, Sergey