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