| Message ID | d9c0acab-909e-da06-decf-be5de59d23bf@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ata_generic: use IS_ENABLED() macro | expand |
On 9/10/24 5:51 AM, Sergey Shtylyov 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. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the for-next branch of the LibATA Group's repo. > > drivers/ata/ata_generic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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... > { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), }, > { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), }, > { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3), }, >
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... >> 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... [...] MBR, Sergey
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 >
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), 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. And obviously, there should be a little interest now in adding the "new" PATA drivers. :-) Any thoughts on the naming confusion? >>>> 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? MBR, Sergey
[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? >>>> 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? MBR, Sergey
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
On 9/11/24 1:22 AM, Damien Le Moal wrote: [...] >> [Resending after adding the missed test, please ignore the previus reply.) Oops, some more typos! :-) [...] >>>>>> 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. I'm sure I typed BAR5 but apparently B went somewhere with further editing... :-) >> 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 If you want to see the original patch: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=33dced2ea5ed03dda10e7f9f41f0910f32e02eaa Some fragments of the patch lived thru the drivers/ide/ removal, see e.g.: https://elixir.bootlin.com/linux/v6.11-rc1/source/drivers/pci/quirks.c#L2319 >> 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... :-) The interesting fact is that the TC86C001 (GOUKU-S) USB device controller (PCI function #2) is still supported by its own driver (drivers/usb/gadget/udc/goku_udc.c), mereg back in 2004... :-) >> Any thoughts on the naming confusion? > > Maybe rename the option to CONFIG_PATA_TOSHIBA_PICCCOLO ? Nah, that doesn't make much sense to me; if we rename it, we should match the driver's name, i.e. make it CONFIG_PATA_PICCOLO. I'm mainly concerned about the Linux distros which would have to handle such rename somehow, IIUC... [...[ MBR, Sergey
On Wed, Sep 11, 2024 at 08:14:32PM +0300, Sergey Shtylyov wrote: > On 9/11/24 1:22 AM, Damien Le Moal wrote: > > > > Maybe rename the option to CONFIG_PATA_TOSHIBA_PICCCOLO ? > > Nah, that doesn't make much sense to me; if we rename it, we should match the driver's name, i.e. make it CONFIG_PATA_PICCOLO. I'm mainly concerned about the > Linux distros which would have to handle such rename somehow, IIUC... I still remember: 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY configuration item") and 55b014159ee7 ("ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back") so I also prefer to avoid renaming Kconfigs as far as possible. Kind regards, Niklas
On 9/13/24 9:57 AM, Niklas Cassel wrote: [...] >>> Maybe rename the option to CONFIG_PATA_TOSHIBA_PICCCOLO ? >> >> Nah, that doesn't make much sense to me; if we rename it, we should match the driver's name, i.e. make it CONFIG_PATA_PICCOLO. I'm mainly concerned about the >> Linux distros which would have to handle such rename somehow, IIUC... I wonder whether they could be using s/th like make allmodconfig... > I still remember: > 4dd4d3deb502 ("ata: ahci: Rename CONFIG_SATA_LPM_MOBILE_POLICY configuration item") > and > 55b014159ee7 ("ata: ahci: Rename CONFIG_SATA_LPM_POLICY configuration item back") > > so I also prefer to avoid renaming Kconfigs as far as possible. Yeah, it does not look very likely ATM that tc86c001 support will be revived... Although, could be done pretty easily if I don't try to work around the infamous limitation #5... > Kind regards, > Niklas MBR, Sergey
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
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) { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), }, { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2), }, { PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_3), },
Replace now gone out of fashion defined(CONFIG_PATA_TOSHIBA[_MODULE]) with the new-fangled IS_ENABLED() macro in the ata_generic[] definition. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the for-next branch of the LibATA Group's repo. drivers/ata/ata_generic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)