Re: [PATCH] ata: ata_generic: use IS_ENABLED() macro

[Damien Le Moal <dlemoal@kernel.org>]

On 2024/09/10 17:52, Sergey Shtylyov wrote:
> On 9/10/24 7:50 AM, Damien Le Moal wrote:
> [...]
> 
>>> Replace now gone out of fashion defined(CONFIG_PATA_TOSHIBA[_MODULE])
>>> with the new-fangled IS_ENABLED() macro in the ata_generic[] definition.
>>
>> Please mention that CONFIG_PATA_TOSHIBA_MODULE actually does not exist at all
>> and so can be removed.
> 
>    Huh? =)
>    CONFIG_PATA_TOSHIBA is a tristate option, so CONFIG_PATA_TOSHIBA_MODULE
> does exist; else there would be no point in using IS_ENABLED() at all...

Oops... Indeed. Got confused with something else :)

>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> [...[
> 
>>> 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 ?

> 
> [...]
> 
> MBR, Sergey
> 

-- 
Damien Le Moal
Western Digital Research