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

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

> -----Original Message-----
> From: Inbaraj E <inbaraj.e@samsung.com>
> Sent: 23 August 2025 17:20
> To: 'Krzysztof Kozlowski' <krzk@kernel.org>; 'mturquette@baylibre.com'
> <mturquette@baylibre.com>; 'sboyd@kernel.org' <sboyd@kernel.org>;
> 'robh@kernel.org' <robh@kernel.org>; 'krzk+dt@kernel.org'
> <krzk+dt@kernel.org>; 'conor+dt@kernel.org' <conor+dt@kernel.org>;
> 's.nawrocki@samsung.com' <s.nawrocki@samsung.com>;
> 's.hauer@pengutronix.de' <s.hauer@pengutronix.de>;
> 'shawnguo@kernel.org' <shawnguo@kernel.org>;
> 'cw00.choi@samsung.com' <cw00.choi@samsung.com>; 'rmfrfs@gmail.com'
> <rmfrfs@gmail.com>; 'laurent.pinchart@ideasonboard.com'
> <laurent.pinchart@ideasonboard.com>; 'martink@posteo.de'
> <martink@posteo.de>; 'mchehab@kernel.org' <mchehab@kernel.org>;
> 'linux-fsd@tesla.com' <linux-fsd@tesla.com>; 'will@kernel.org'
> <will@kernel.org>; 'catalin.marinas@arm.com' <catalin.marinas@arm.com>;
> 'pankaj.dubey@samsung.com' <pankaj.dubey@samsung.com>;
> 'shradha.t@samsung.com' <shradha.t@samsung.com>;
> 'ravi.patel@samsung.com' <ravi.patel@samsung.com>
> Cc: 'linux-clk@vger.kernel.org' <linux-clk@vger.kernel.org>;
> 'devicetree@vger.kernel.org' <devicetree@vger.kernel.org>; 'linux-
> kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>;
> 'alim.akhtar@samsung.com' <alim.akhtar@samsung.com>; 'linux-samsung-
> soc@vger.kernel.org' <linux-samsung-soc@vger.kernel.org>;
> 'kernel@puri.sm' <kernel@puri.sm>; 'kernel@pengutronix.de'
> <kernel@pengutronix.de>; 'festevam@gmail.com' <festevam@gmail.com>;
> 'linux-media@vger.kernel.org' <linux-media@vger.kernel.org>;
> 'imx@lists.linux.dev' <imx@lists.linux.dev>; 'linux-arm-
> kernel@lists.infradead.org' <linux-arm-kernel@lists.infradead.org>
> Subject: RE: [PATCH v2 12/12] media: fsd-csis: Add support for FSD CSIS DMA
> 
> 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?
> 

Sorry I made typo here

 If CONFIG_PM is disabled, 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