> -----Original Message----- > From: Inbaraj E <inbaraj.e@xxxxxxxxxxx> > Sent: 23 August 2025 17:20 > To: 'Krzysztof Kozlowski' <krzk@xxxxxxxxxx>; 'mturquette@xxxxxxxxxxxx' > <mturquette@xxxxxxxxxxxx>; 'sboyd@xxxxxxxxxx' <sboyd@xxxxxxxxxx>; > 'robh@xxxxxxxxxx' <robh@xxxxxxxxxx>; 'krzk+dt@xxxxxxxxxx' > <krzk+dt@xxxxxxxxxx>; 'conor+dt@xxxxxxxxxx' <conor+dt@xxxxxxxxxx>; > 's.nawrocki@xxxxxxxxxxx' <s.nawrocki@xxxxxxxxxxx>; > 's.hauer@xxxxxxxxxxxxxx' <s.hauer@xxxxxxxxxxxxxx>; > 'shawnguo@xxxxxxxxxx' <shawnguo@xxxxxxxxxx>; > 'cw00.choi@xxxxxxxxxxx' <cw00.choi@xxxxxxxxxxx>; 'rmfrfs@xxxxxxxxx' > <rmfrfs@xxxxxxxxx>; 'laurent.pinchart@xxxxxxxxxxxxxxxx' > <laurent.pinchart@xxxxxxxxxxxxxxxx>; 'martink@xxxxxxxxx' > <martink@xxxxxxxxx>; 'mchehab@xxxxxxxxxx' <mchehab@xxxxxxxxxx>; > 'linux-fsd@xxxxxxxxx' <linux-fsd@xxxxxxxxx>; 'will@xxxxxxxxxx' > <will@xxxxxxxxxx>; 'catalin.marinas@xxxxxxx' <catalin.marinas@xxxxxxx>; > 'pankaj.dubey@xxxxxxxxxxx' <pankaj.dubey@xxxxxxxxxxx>; > 'shradha.t@xxxxxxxxxxx' <shradha.t@xxxxxxxxxxx>; > 'ravi.patel@xxxxxxxxxxx' <ravi.patel@xxxxxxxxxxx> > Cc: 'linux-clk@xxxxxxxxxxxxxxx' <linux-clk@xxxxxxxxxxxxxxx>; > 'devicetree@xxxxxxxxxxxxxxx' <devicetree@xxxxxxxxxxxxxxx>; 'linux- > kernel@xxxxxxxxxxxxxxx' <linux-kernel@xxxxxxxxxxxxxxx>; > 'alim.akhtar@xxxxxxxxxxx' <alim.akhtar@xxxxxxxxxxx>; 'linux-samsung- > soc@xxxxxxxxxxxxxxx' <linux-samsung-soc@xxxxxxxxxxxxxxx>; > 'kernel@xxxxxxx' <kernel@xxxxxxx>; 'kernel@xxxxxxxxxxxxxx' > <kernel@xxxxxxxxxxxxxx>; 'festevam@xxxxxxxxx' <festevam@xxxxxxxxx>; > 'linux-media@xxxxxxxxxxxxxxx' <linux-media@xxxxxxxxxxxxxxx>; > 'imx@xxxxxxxxxxxxxxx' <imx@xxxxxxxxxxxxxxx>; 'linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx' <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx> > 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@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > > +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non- > > subscribers) > > > > Replace above list with samsung-soc list. > > > > I'll change in next patchset. > > > > +L: linux-media@xxxxxxxxxxxxxxx > > > +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
