RE: [PATCH v2 12/12] media: fsd-csis: Add support for FSD CSIS DMA

["Inbaraj E" <inbaraj.e@samsung.com>]

Hi Krzysztof,

Thanks for the review.

> 
> On 14/08/2025 16:09, Inbaraj E wrote:
> > FSD CSIS IP bundles DMA engine for receiving frames from MIPI-CSI2 bus.
> > Add support internal DMA controller to capture the frames.
> >
> > Signed-off-by: Inbaraj E <inbaraj.e@samsung.com>
> 
> I commented on order of patches and got more surprise - final driver patch
> after DTS defconfig. It's really wrong order.

I'll fix in next patchset.

> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3334,6 +3334,14 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/media/samsung,s5p-mfc.yaml
> >  F:	drivers/media/platform/samsung/s5p-mfc/
> >
> > +ARM/SAMSUNG FSD BRIDGE DRIVER
> 
> TESLA FSD BRIDGE DRIVER
> (because ARM/foo are only SoC maintainer entries)
> 

I'll change in next patchset.

> > +M:	Inbaraj E <inbaraj.e@samsung.com>
> > +L:	linux-arm-kernel@lists.infradead.org (moderated for non-
> subscribers)
> 
> Replace above list with samsung-soc list.
> 

I'll change in next patchset.
 
> > +L:	linux-media@vger.kernel.org
> > +S:	Maintained
> >  source "drivers/media/platform/samsung/exynos-gsc/Kconfig"
> >  source "drivers/media/platform/samsung/exynos4-is/Kconfig"
> > +
> > +config VIDEO_FSD_CSIS
> 
> VIDEO_TSLA_FSD_CSIS

I'll change in next patchset.

> 
> > +	tristate "FSD SoC MIPI-CSI2 media controller driver"
> > +	depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API
> > +	depends on HAS_DMA
> > +	depends on OF
> 
> OF seems unneeded dependency
> 
> But you miss ARCH_TESLA_FSD instead.
> 
> 

I'll remove OF and add ARCH_TESLA_FSD in next patchset.

> > +	select VIDEOBUF2_DMA_CONTIG
> > +	select V4L2_FWNODE
> > +	help
> > +	  This is a video4linux2 driver for FSD SoC MIPI-CSI2 Rx.
> 
> Tesla FSD

I'll add in next patchset.

> > new file mode 100644
> > index 000000000000..74f46038d506
> > --- /dev/null
> > +++ b/drivers/media/platform/samsung/fsd-csis/fsd-csis.c
> > @@ -0,0 +1,1709 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022-2025 Samsung Electronics Co., Ltd.
> > + *             https://www.samsung.com
> > + *
> > + * FSD CSIS V4L2 Capture driver for FSD SoC.
> 
> "Tesla FSD" in both places

I'll change in next patchset.

> 
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-ioctl.h>
> > +#include <media/videobuf2-dma-contig.h> #include <media/v4l2-mc.h>
> 
> How can you depend on OF if there is no single OF header?

> > +
> > +	ret = devm_request_irq(dev, irq,
> > +			csis_irq_handler, IRQF_SHARED, pdev->name, csis);
> 
> Please align these (checkpatch --strict)

I'll fix in next patchset

> 
> > +
> > +	ret = fsd_csis_clk_get(csis);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pm_runtime_enable(dev);
> > +	if (!pm_runtime_enabled(dev)) {
> 
> That's odd code. Why?
> 
> > +		ret = fsd_csis_runtime_resume(dev);
> 
> Even more questions why?

If CONFIG_PM is enabled, the clocks are enabled manually in the
driver through fsd_csis_runtime_resume API.

If CONFIG_PM is enabled, the clocks are managed through the PM
runtime framework.

Can you please help me understand what wrong here?

> 
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, csis);
> > +
> > +	ret = fsd_csis_enable_pll(csis);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = fsd_csis_media_init(csis);
> > +	if (ret)
> > +		return ret;
> 
> I think you miss clean up of csis->pll completely. Just use
> devm_clk_get_enabled and convert everything here to devm.
> 
> 

I'll fix in next patchset.

> > +
> > +	ret = fsd_csis_async_register(csis);
> > +	if (ret)
> > +		goto err_media_cleanup;
> > +
> > +	return 0;
> > +
> > +err_media_cleanup:
> > +	fsd_csis_media_cleanup(csis);
> 
> Also this...
> 

if fsd_csis_media_init fails, the cleanup is handled internally.
Here, cleanup is used only for fsd_csis_async_register failure.

can you please help me understand what is wrong here?
 
> > +
> > +	return ret;
> > +}
> > +
> > +static void fsd_csis_remove(struct platform_device *pdev) {
> > +	struct fsd_csis *csis = platform_get_drvdata(pdev);
> > +
> > +static struct platform_driver fsd_csis_driver = {
> > +	.probe		= fsd_csis_probe,
> > +	.remove		= fsd_csis_remove,
> > +	.driver		= {
> > +		.name		= FSD_CSIS_MODULE_NAME,
> > +		.of_match_table = of_match_ptr(fsd_csis_of_match),
> 
> Drop of_match_ptr, it is not really correct.

Will drop in next patchset.

Regards,
Inbaraj E