diff mbox series

[2/2] media: pci: mgb4: remove bogus 'select' statements

Message ID 20231023160539.1537355-2-arnd@kernel.org
State New
Headers show
Series [1/2] media: pci: mgb4: add COMMON_CLK dependency | expand

Commit Message

Arnd Bergmann Oct. 23, 2023, 4:05 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

As this is just a regular device driver, it has no business force-enabling
other drivers in the system, it should be entirely independent of the
implementation of the spi-nor layer or the specific DMA engine.

The IIO symbols that are selected here are library modules that
are legitimately used.

Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/pci/mgb4/Kconfig | 4 ----
 1 file changed, 4 deletions(-)

Comments

Arnd Bergmann Nov. 8, 2023, 4:13 p.m. UTC | #1
On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>
> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> As this is just a regular device driver, it has no business force-enabling
>> other drivers in the system, it should be entirely independent of the
>> implementation of the spi-nor layer or the specific DMA engine.
>> 
>
> The drivers are required for IP cores that are used on the card (in the 
> FPGA). Without I2C_XILINX and XILINX_XDMA the card won't work at all. 
> Without SPI_XILINX the access to the card's FLASH (used e.g. for FW 
> changes) won't be possible.
>
> A change to "depend" instead of "select" is thus possible if it makes 
> more sense to you, but removing it would make the module not compile or 
> not work at runtime (there is no symbol dependency to I2C_XILINX and 
> SPI_XILINX, but both need to be present and are loaded using 
> request_module() at runtime).

Sorry for the delay at getting back to you here.

I don't think there is a good answer here, though I normally
try to only list the minimal dependencies that are required
at build time. E.g. for on-chip devices we don't require the
use of a particular clock/irq/pin/gpio/... controller even if
we know exactly which of those are used on a given chip.

Since this is a PCI device, it's a bit different, so maybe
something like this would work to correctly document which
dependencies are required at build time vs run time:

--- a/drivers/media/pci/mgb4/Kconfig
+++ b/drivers/media/pci/mgb4/Kconfig
@@ -1,15 +1,13 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config VIDEO_MGB4
        tristate "Digiteq Automotive MGB4 support"
-       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
+       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
        depends on COMMON_CLK
+       depends on XILINX_XDMA
+       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
        select VIDEOBUF2_DMA_SG
        select IIO_BUFFER
        select IIO_TRIGGERED_BUFFER
-       select I2C_XILINX
-       select SPI_XILINX
-       select MTD_SPI_NOR
-       select XILINX_XDMA
        help
          This is a video4linux driver for Digiteq Automotive MGB4 grabber
          cards.

     Arnd
Martin Tůma Nov. 8, 2023, 6:33 p.m. UTC | #2
On 08. 11. 23 17:13, Arnd Bergmann wrote:
> On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>>
>> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> As this is just a regular device driver, it has no business force-enabling
>>> other drivers in the system, it should be entirely independent of the
>>> implementation of the spi-nor layer or the specific DMA engine.
>>>
>>
>> The drivers are required for IP cores that are used on the card (in the
>> FPGA). Without I2C_XILINX and XILINX_XDMA the card won't work at all.
>> Without SPI_XILINX the access to the card's FLASH (used e.g. for FW
>> changes) won't be possible.
>>
>> A change to "depend" instead of "select" is thus possible if it makes
>> more sense to you, but removing it would make the module not compile or
>> not work at runtime (there is no symbol dependency to I2C_XILINX and
>> SPI_XILINX, but both need to be present and are loaded using
>> request_module() at runtime).
> 
> Sorry for the delay at getting back to you here.
> 
> I don't think there is a good answer here, though I normally
> try to only list the minimal dependencies that are required
> at build time. E.g. for on-chip devices we don't require the
> use of a particular clock/irq/pin/gpio/... controller even if
> we know exactly which of those are used on a given chip.
> 

On SoCs you probably get a kernel configuration that is missing some 
feature but still boots up when you do not select/depend on the exact 
controller, but in the case of the mgb4 PCIe card you get a driver that 
does not work at all (The SPI_XILINX dependency could theoretically be 
made configurable, but you would lose the ability to flash the correct 
FW for the current HW module and the access to the card's serial number. 
I2C and XDMA are crucial.).

> Since this is a PCI device, it's a bit different, so maybe
> something like this would work to correctly document which
> dependencies are required at build time vs run time:
> 
> --- a/drivers/media/pci/mgb4/Kconfig
> +++ b/drivers/media/pci/mgb4/Kconfig
> @@ -1,15 +1,13 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   config VIDEO_MGB4
>          tristate "Digiteq Automotive MGB4 support"
> -       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
> +       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
>          depends on COMMON_CLK
> +       depends on XILINX_XDMA
> +       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
>          select VIDEOBUF2_DMA_SG
>          select IIO_BUFFER
>          select IIO_TRIGGERED_BUFFER
> -       select I2C_XILINX
> -       select SPI_XILINX
> -       select MTD_SPI_NOR
> -       select XILINX_XDMA
>          help
>            This is a video4linux driver for Digiteq Automotive MGB4 grabber
>            cards.
> 

My motivation when using "select" was to help people using "make 
menuconfig" to get the module selected/configured as they will usually 
not know that there are some Xilinx IP cores used that need separate 
drivers and the menuconfig GUI simply hides the mgb4 option making it 
almost impossible just from the menus to find out what has to be selected.

But when there are reasons, why to chose "depends on" (like various 
configurations, tests or the "readability" of the dependencies) than I'm 
ok with your patch proposal.

M.

>       Arnd
Arnd Bergmann Nov. 10, 2023, 2:37 p.m. UTC | #3
On Wed, Nov 8, 2023, at 19:33, Martin Tůma wrote:
> On 08. 11. 23 17:13, Arnd Bergmann wrote:
>> On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>>> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>
> On SoCs you probably get a kernel configuration that is missing some 
> feature but still boots up when you do not select/depend on the exact 
> controller, but in the case of the mgb4 PCIe card you get a driver that 
> does not work at all (The SPI_XILINX dependency could theoretically be 
> made configurable, but you would lose the ability to flash the correct 
> FW for the current HW module and the access to the card's serial number. 
> I2C and XDMA are crucial.).

My point was that we do this all the time for things that are
essential: if your clock controller or the irqchip have
no driver, then the camera device won't work, but neither
would anything else.

So in a SoC setting, you really just need to enable all
the drivers for devices on that chip through the .config.

>> Since this is a PCI device, it's a bit different, so maybe
>> something like this would work to correctly document which
>> dependencies are required at build time vs run time:
>> 
>> --- a/drivers/media/pci/mgb4/Kconfig
>> +++ b/drivers/media/pci/mgb4/Kconfig
>> @@ -1,15 +1,13 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   config VIDEO_MGB4
>>          tristate "Digiteq Automotive MGB4 support"
>> -       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
>> +       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
>>          depends on COMMON_CLK
>> +       depends on XILINX_XDMA
>> +       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
>>          select VIDEOBUF2_DMA_SG
>>          select IIO_BUFFER
>>          select IIO_TRIGGERED_BUFFER
>> -       select I2C_XILINX
>> -       select SPI_XILINX
>> -       select MTD_SPI_NOR
>> -       select XILINX_XDMA
>>          help
>>            This is a video4linux driver for Digiteq Automotive MGB4 grabber
>>            cards.
>> 
>
> My motivation when using "select" was to help people using "make 
> menuconfig" to get the module selected/configured as they will usually 
> not know that there are some Xilinx IP cores used that need separate 
> drivers and the menuconfig GUI simply hides the mgb4 option making it 
> almost impossible just from the menus to find out what has to be selected.
>
> But when there are reasons, why to chose "depends on" (like various 
> configurations, tests or the "readability" of the dependencies) than I'm 
> ok with your patch proposal.

The main reason to use 'depends on' over 'select' here is that
mixing the two is a common source of dependency loops that end
up breaking the build. As a rule of thumb, I would use 'select'
only for symbols that others already select, or that are hidden
from visibility.

      Arnd
Hans Verkuil Nov. 13, 2023, 2:57 p.m. UTC | #4
On 10/11/2023 15:37, Arnd Bergmann wrote:
> On Wed, Nov 8, 2023, at 19:33, Martin Tůma wrote:
>> On 08. 11. 23 17:13, Arnd Bergmann wrote:
>>> On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>>>> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> On SoCs you probably get a kernel configuration that is missing some 
>> feature but still boots up when you do not select/depend on the exact 
>> controller, but in the case of the mgb4 PCIe card you get a driver that 
>> does not work at all (The SPI_XILINX dependency could theoretically be 
>> made configurable, but you would lose the ability to flash the correct 
>> FW for the current HW module and the access to the card's serial number. 
>> I2C and XDMA are crucial.).
> 
> My point was that we do this all the time for things that are
> essential: if your clock controller or the irqchip have
> no driver, then the camera device won't work, but neither
> would anything else.
> 
> So in a SoC setting, you really just need to enable all
> the drivers for devices on that chip through the .config.
> 
>>> Since this is a PCI device, it's a bit different, so maybe
>>> something like this would work to correctly document which
>>> dependencies are required at build time vs run time:
>>>
>>> --- a/drivers/media/pci/mgb4/Kconfig
>>> +++ b/drivers/media/pci/mgb4/Kconfig
>>> @@ -1,15 +1,13 @@
>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>   config VIDEO_MGB4
>>>          tristate "Digiteq Automotive MGB4 support"
>>> -       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
>>> +       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
>>>          depends on COMMON_CLK
>>> +       depends on XILINX_XDMA
>>> +       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
>>>          select VIDEOBUF2_DMA_SG
>>>          select IIO_BUFFER
>>>          select IIO_TRIGGERED_BUFFER
>>> -       select I2C_XILINX
>>> -       select SPI_XILINX
>>> -       select MTD_SPI_NOR
>>> -       select XILINX_XDMA
>>>          help
>>>            This is a video4linux driver for Digiteq Automotive MGB4 grabber
>>>            cards.
>>>
>>
>> My motivation when using "select" was to help people using "make 
>> menuconfig" to get the module selected/configured as they will usually 
>> not know that there are some Xilinx IP cores used that need separate 
>> drivers and the menuconfig GUI simply hides the mgb4 option making it 
>> almost impossible just from the menus to find out what has to be selected.
>>
>> But when there are reasons, why to chose "depends on" (like various 
>> configurations, tests or the "readability" of the dependencies) than I'm 
>> ok with your patch proposal.
> 
> The main reason to use 'depends on' over 'select' here is that
> mixing the two is a common source of dependency loops that end
> up breaking the build. As a rule of thumb, I would use 'select'
> only for symbols that others already select, or that are hidden
> from visibility.
> 
>       Arnd
> 

Arnd, can you post a v2? I think this should go to v6.7 as a fix.

Regards,

	Hans
Hans Verkuil Dec. 6, 2023, 9:18 a.m. UTC | #5
Arnd,

Can you post a v2 for this?

Note that patch 1/1 is in v6.7 already.

Regards,

	Hans

On 13/11/2023 15:57, Hans Verkuil wrote:
> On 10/11/2023 15:37, Arnd Bergmann wrote:
>> On Wed, Nov 8, 2023, at 19:33, Martin Tůma wrote:
>>> On 08. 11. 23 17:13, Arnd Bergmann wrote:
>>>> On Fri, Oct 27, 2023, at 16:17, Martin Tůma wrote:
>>>>> On 23. 10. 23 18:05, Arnd Bergmann wrote:
>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> On SoCs you probably get a kernel configuration that is missing some 
>>> feature but still boots up when you do not select/depend on the exact 
>>> controller, but in the case of the mgb4 PCIe card you get a driver that 
>>> does not work at all (The SPI_XILINX dependency could theoretically be 
>>> made configurable, but you would lose the ability to flash the correct 
>>> FW for the current HW module and the access to the card's serial number. 
>>> I2C and XDMA are crucial.).
>>
>> My point was that we do this all the time for things that are
>> essential: if your clock controller or the irqchip have
>> no driver, then the camera device won't work, but neither
>> would anything else.
>>
>> So in a SoC setting, you really just need to enable all
>> the drivers for devices on that chip through the .config.
>>
>>>> Since this is a PCI device, it's a bit different, so maybe
>>>> something like this would work to correctly document which
>>>> dependencies are required at build time vs run time:
>>>>
>>>> --- a/drivers/media/pci/mgb4/Kconfig
>>>> +++ b/drivers/media/pci/mgb4/Kconfig
>>>> @@ -1,15 +1,13 @@
>>>>   # SPDX-License-Identifier: GPL-2.0-only
>>>>   config VIDEO_MGB4
>>>>          tristate "Digiteq Automotive MGB4 support"
>>>> -       depends on VIDEO_DEV && PCI && I2C && DMADEVICES && SPI && MTD && IIO
>>>> +       depends on VIDEO_DEV && PCI && I2C && SPI && MTD && IIO
>>>>          depends on COMMON_CLK
>>>> +       depends on XILINX_XDMA
>>>> +       depends on (I2C_XILINX && SPI_XILINX) || COMPILE_TEST
>>>>          select VIDEOBUF2_DMA_SG
>>>>          select IIO_BUFFER
>>>>          select IIO_TRIGGERED_BUFFER
>>>> -       select I2C_XILINX
>>>> -       select SPI_XILINX
>>>> -       select MTD_SPI_NOR
>>>> -       select XILINX_XDMA
>>>>          help
>>>>            This is a video4linux driver for Digiteq Automotive MGB4 grabber
>>>>            cards.
>>>>
>>>
>>> My motivation when using "select" was to help people using "make 
>>> menuconfig" to get the module selected/configured as they will usually 
>>> not know that there are some Xilinx IP cores used that need separate 
>>> drivers and the menuconfig GUI simply hides the mgb4 option making it 
>>> almost impossible just from the menus to find out what has to be selected.
>>>
>>> But when there are reasons, why to chose "depends on" (like various 
>>> configurations, tests or the "readability" of the dependencies) than I'm 
>>> ok with your patch proposal.
>>
>> The main reason to use 'depends on' over 'select' here is that
>> mixing the two is a common source of dependency loops that end
>> up breaking the build. As a rule of thumb, I would use 'select'
>> only for symbols that others already select, or that are hidden
>> from visibility.
>>
>>       Arnd
>>
> 
> Arnd, can you post a v2? I think this should go to v6.7 as a fix.
> 
> Regards,
> 
> 	Hans
>
diff mbox series

Patch

diff --git a/drivers/media/pci/mgb4/Kconfig b/drivers/media/pci/mgb4/Kconfig
index f2a05a1c8ffa..b90347c7f19b 100644
--- a/drivers/media/pci/mgb4/Kconfig
+++ b/drivers/media/pci/mgb4/Kconfig
@@ -6,10 +6,6 @@  config VIDEO_MGB4
 	select VIDEOBUF2_DMA_SG
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
-	select I2C_XILINX
-	select SPI_XILINX
-	select MTD_SPI_NOR
-	select XILINX_XDMA
 	help
 	  This is a video4linux driver for Digiteq Automotive MGB4 grabber
 	  cards.