Message ID | 20231023160539.1537355-2-arnd@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce89:0:b0:403:3b70:6f57 with SMTP id p9csp1397224vqx; Mon, 23 Oct 2023 09:06:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHxNEu8hiIcXtAwGgE9Q90B5+HmahHNiwWf0r6BtwT9ELdAL7PTUf9RVUoRRBv31Pc+BjxC X-Received: by 2002:aa7:900b:0:b0:692:6417:728a with SMTP id m11-20020aa7900b000000b006926417728amr15782400pfo.14.1698077192458; Mon, 23 Oct 2023 09:06:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698077192; cv=none; d=google.com; s=arc-20160816; b=araqyjiHjyrrD5JQMlnEmHu662gx+zFJSVUdojTtZHdeoVhFy1hxF9bG0XxwWOU+YS 9HPH8ug5yA5u1SRYNcqqYw3U2A3GYbkKKDoOdUXOeD5fpXMiHfnRVltzIdLx5G2HRdPH 7aOoheRSvJtE1ey890+OH6qGb2bhJd45CyU6WyoQXgukjBaqCd8adV/Q4v99q76SX6Ws rpYdplR0PfmI5FW7SkC08UKjGQQE2LiKPk3dwKz6Q668TyVPcnCqvT4Vb1M0MJ1osDDb 1NuSVvE9NCXEw3TUbx9p4VRG4drf7O4QL2GeecP7wzQ2YCTOw+PjcVO8bvMB239rJRVM SlpA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=Bz87jYUzLh+FX65i0DPntoGJ5vgU1vVglGkHGZGfs3k=; fh=PU2XAJLNWGeVyB4Y7WsAvzTXOGVtgVL6qGs3oEaf7AE=; b=SHNjrD6INrwvlhPoa/rQ9cvWh2jOXR2JzcZVfBFTCbIivM3HQ4fxeo9vI0PmbHd0Yj KLplSvMqdg4PzWh+XcFTA8sQ4DIsF3onWvoFwbJGg89xZRNqe2UU77/kNkZpVB2l/ArV Bw9szujU50qh+ygM70JkWtr5geNh8GW5eKKQYJBIyeKShjtGGPUSek5JuzrslDBf7dOh dNph0BH/90/gev+MnthgSCq6cociuqN+3nBXZPO79SPMkT2T8cfNMlgUs73JJQ7HZDi3 tIXEWwn2qZ2gEaZz3FxczBY2WTkGgLf66auTVRfD5DeuXo/2rXrGVkHOCNCCvTHJA1Q4 Zj/w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=CDIxQ4aX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id d17-20020a637351000000b0058986c3dc2fsi6441220pgn.117.2023.10.23.09.06.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Oct 2023 09:06:32 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=CDIxQ4aX; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id B96B580657E2; Mon, 23 Oct 2023 09:06:08 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232185AbjJWQF6 (ORCPT <rfc822;aposhian.dev@gmail.com> + 27 others); Mon, 23 Oct 2023 12:05:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44420 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232454AbjJWQFy (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 23 Oct 2023 12:05:54 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1585910D; Mon, 23 Oct 2023 09:05:53 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 41DB4C433C8; Mon, 23 Oct 2023 16:05:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1698077152; bh=rsdBpDp3RXcMnY1Y8bJLNbH0IoXz+q0JDXqgZCh8R3o=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CDIxQ4aXY28mYhgeBoKUkAaWIKehUEhKwHGVdlqPlHjBx8ZACUP0ilYGo5EUNmauI 7oArSePao6D3uH3ld0IL84YcNO1B4VWzszoiT0pPsKsdNEfgyvYAEu0P5npvOGyAuw 6DmfRN9QEH1FUMkbYAuybaOozq0EliKV3SSODPhDBePi21UoncKecE8nsTyKXW8uMv hOtykMbEFxhkg95hTh6w1O7R5MWHz2JyPCMn1UnfZcUApeqBl1CiYwTPR2dkSEvy+d +J31ChpnJOBzoexxVqNKx3pzpSvvRlnivjAOQJ7pwlCo/Qre9OKpyk9tNyTUkMloB3 d4pjfvyLsgxYA== From: Arnd Bergmann <arnd@kernel.org> To: Martin Tuma <martin.tuma@digiteqautomotive.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Hans Verkuil <hverkuil-cisco@xs4all.nl> Cc: Arnd Bergmann <arnd@arndb.de>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] media: pci: mgb4: remove bogus 'select' statements Date: Mon, 23 Oct 2023 18:05:32 +0200 Message-Id: <20231023160539.1537355-2-arnd@kernel.org> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231023160539.1537355-1-arnd@kernel.org> References: <20231023160539.1537355-1-arnd@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Mon, 23 Oct 2023 09:06:08 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780562989894512674 X-GMAIL-MSGID: 1780562989894512674 |
Series |
[1/2] media: pci: mgb4: add COMMON_CLK dependency
|
|
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
On Mon, Oct 23, 2023, at 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 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(-) > > 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 Apparently, the XDMA reference was in fact needed, as MGB4 calls some exported symbols from that particular dmaengine driver: aarch64-linux-ld: drivers/media/pci/mgb4/mgb4_core.o: in function `init_i2c': mgb4_core.c:(.text+0x3ec): undefined reference to `xdma_get_user_irq' aarch64-linux-ld: mgb4_core.c:(.text+0x404): undefined reference to `xdma_enable_user_irq' I couldn't easily figure out what a 'user_irq' is here, but I wonder if this is the expected way to use the DMA engine layer. Maybe this should have been a nested irqchip instead, or it should be encoded in the DMA request specifier? Arnd
On 10/24/23 06:27, Arnd Bergmann wrote: > On Mon, Oct 23, 2023, at 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 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(-) >> >> 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 > Apparently, the XDMA reference was in fact needed, as MGB4 > calls some exported symbols from that particular dmaengine > driver: > > aarch64-linux-ld: drivers/media/pci/mgb4/mgb4_core.o: in function `init_i2c': mgb4_core.c:(.text+0x3ec): undefined reference to `xdma_get_user_irq' > aarch64-linux-ld: mgb4_core.c:(.text+0x404): undefined reference to `xdma_enable_user_irq' > > I couldn't easily figure out what a 'user_irq' is here, > but I wonder if this is the expected way to use the DMA engine > layer. Maybe this should have been a nested irqchip instead, > or it should be encoded in the DMA request specifier? Hi Arnd, Here is a brief description of 'user_irq' and 'xdma_enable_user_irq' The XDMA subsystem is used in conjunction with the PCIe IP block. Please see https://lwn.net/Articles/911496/ for the overall information. XDMA can forward PCIe msi-x interrupt to/from user logic hardware (e.g. Digiteq device) which is connected to its user irq pin. And XDMA has a register to enable/disabe interrupt forwarding for a specific user irq pin. 'xdma_enable_user_irq' and 'xdma_disable_user_irq' are provided for hardware driver which is designed to use XDMA to enable/disable its interrupt. And based on the previous discussion with Mark, Digiteq device does not use its own register to enable/disable interrupt but relies on XDMA. Please see https://lore.kernel.org/lkml/daccee4a-ac3c-bfc1-4876-24e6ecf5bcf1@gpxsee.org/ Thanks, Lizhi > > Arnd
On Tue, Oct 24, 2023, at 18:18, Lizhi Hou wrote: > On 10/24/23 06:27, Arnd Bergmann wrote: >> On Mon, Oct 23, 2023, at 18:05, Arnd Bergmann wrote: >> >> aarch64-linux-ld: drivers/media/pci/mgb4/mgb4_core.o: in function `init_i2c': mgb4_core.c:(.text+0x3ec): undefined reference to `xdma_get_user_irq' >> aarch64-linux-ld: mgb4_core.c:(.text+0x404): undefined reference to `xdma_enable_user_irq' >> >> I couldn't easily figure out what a 'user_irq' is here, >> but I wonder if this is the expected way to use the DMA engine >> layer. Maybe this should have been a nested irqchip instead, >> or it should be encoded in the DMA request specifier? > > Hi Arnd, > > Here is a brief description of 'user_irq' and 'xdma_enable_user_irq' > > The XDMA subsystem is used in conjunction with the PCIe IP block. Please > see https://lwn.net/Articles/911496/ for the overall information. > > XDMA can forward PCIe msi-x interrupt to/from user logic hardware (e.g. > Digiteq device) which is connected to its user irq pin. And XDMA has a > register to enable/disabe interrupt forwarding for a specific user irq pin. > > 'xdma_enable_user_irq' and 'xdma_disable_user_irq' are provided for > hardware driver which is designed to use XDMA to enable/disable its > interrupt. And based on the previous discussion with Mark, Digiteq > device does not use its own register to enable/disable interrupt but > relies on XDMA. Please see > https://lore.kernel.org/lkml/daccee4a-ac3c-bfc1-4876-24e6ecf5bcf1@gpxsee.org/ Ok, in this case, I would suggest using 'depends on XILINX_XDMA' instead of the 'select' statement. I'll send a v2. Arnd
Hi, 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). M. > 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(-) > > 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.
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
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
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
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
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 --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.