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

[Sergey Shtylyov <s.shtylyov@omp.ru>]

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@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 ?
>>
>>    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