On 1/10/2025 7:58 PM, Johan Hovold wrote: > On Thu, Jan 09, 2025 at 11:18:29PM +0530, Vikash Garodia wrote: >> On 1/9/2025 8:41 PM, Johan Hovold wrote: >>> On Thu, Dec 12, 2024 at 05:21:49PM +0530, Dikshita Agarwal wrote: >>>> Initialize the platform data and enable video driver probe of SM8250 >>>> SoC. Add a kernel param to select between venus and iris drivers for >>>> platforms supported by both drivers, for ex: SM8250. >>> >>> Why do you want to use a module parameter for this? What would be the >>> default configuration? (Module parameters should generally be avoided.) > >> This was discussed during v4 [1] and implemented as per suggestion >> >> [1] >> https://lore.kernel.org/linux-media/eea14133-2152-37bb-e2ff-fcc7ed4c47f5@xxxxxxxxxxx/ > > First, the background and motivation for this still needs to go in the > commit message (and be mentioned in the cover letter). > > Second, what you implemented here is not even equivalent to what was > done in the mdm drm driver since that module parameter is honoured by > both drivers so that at most one driver tries to bind to the platform > device. > > With this patch as it stands, which driver ends up binding depends on > things like link order and what driver has been built a module, etc. (as > I pointed out below). > >>> Why not simply switch to the new driver (and make sure that the new >>> driver is selected if the old one was enabled in the kernel config)? > >> Its about the platform in migration i.e sm8250. Since new driver is not yet >> feature parity with old driver, choice is provided to client if it wants to use >> the new driver (default being old driver for sm8250) > > This should be described in the commit message, along with details on > what the delta is so that the reasoning can be evaluated. > > And I'm still not sure using a module parameter for this is the right > thing to do as it is generally something that should be avoided. > I understand your concern of using module params. I will modify it to rely on Kconfig to select the driver (suggested by Hans) instead of module param. something like: config VIDEO_QCOM_IRIS tristate "Qualcomm iris V4L2 decoder driver" ... depends on VIDEO_QCOM_VENUS=n || COMPILE_TEST Thanks, Dikshita >>>> static int iris_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -196,6 +224,9 @@ static int iris_probe(struct platform_device *pdev) >>>> u64 dma_mask; >>>> int ret; >>>> >>>> + if (!video_drv_should_bind(&pdev->dev, true)) >>>> + return -ENODEV; >>> >>> AFAICT nothing is preventing venus from binding even when 'prefer_venus' >>> is false. >>> >>>> + >>>> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >>>> if (!core) >>>> return -ENOMEM; > > Johan
