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

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

On 9/10/24 23:36, Sergey Shtylyov wrote:
> [Resending after adding the missed test, please ignore the previus reply.)
> 
> On 9/10/24 4:09 PM, Damien Le Moal wrote:
> [...]
> 
>>>>> Replace now gone out of fashion defined(CONFIG_PATA_TOSHIBA[_MODULE])
> 
>    I'll probably rephrase this a bit in v2...
> 
>>>>> 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 :)
> 
>    There's something to be confused about this driver vs its Kconfig option
> naming: the driver is called pata_piccolo.c and its option CONFIG_PATA_TOSHIBA.
> However, Toshiba seemingly has more than one family of the PATA controllers:
> there's also TC86C001 PCI multi-function chip (dubbed GOKU-S by Toshiba) which
> supports up to UDMA66 and doesn't seem compatible with Piccolo, judging by the
> driver code and Toshiba GOKU-S datasheet I have: the timing regs are mapped @
> AR5 and not in the PCI config space, like with the Piccolo chips.
>    If somebody like me (it was me who submitted the reworked Toshiba's TC86C001
> driver for drivers/ide/ back in 2007) added TC86C001 libata driver, the confusion
> would probably worsen... :-/ Luckily, the chip is a bit tricky (I had to somewhat
> abuse drivers/ide/ to work around some "limitations", as Toshiba calls their errata)
> and I don't have access to the chip to properly test the driver anymore.  Obviously, there should be a little interest now in adding the "new" PATA drivers... :-)
>    Any thoughts on the naming confusion?

Maybe rename the option to CONFIG_PATA_TOSHIBA_PICCCOLO ?

> 
>>>>> 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.

> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research