| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <00e301dc1424$033ed5a0$09bc80e0$@samsung.com>
Date: Sat, 23 Aug 2025 17:19:36 +0530
From: "Inbaraj E" <inbaraj.e@...sung.com>
To: "'Krzysztof Kozlowski'" <krzk@...nel.org>, <mturquette@...libre.com>,
<sboyd@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <s.nawrocki@...sung.com>, <s.hauer@...gutronix.de>,
<shawnguo@...nel.org>, <cw00.choi@...sung.com>, <rmfrfs@...il.com>,
<laurent.pinchart@...asonboard.com>, <martink@...teo.de>,
<mchehab@...nel.org>, <linux-fsd@...la.com>, <will@...nel.org>,
<catalin.marinas@....com>, <pankaj.dubey@...sung.com>,
<shradha.t@...sung.com>, <ravi.patel@...sung.com>
Cc: <linux-clk@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <alim.akhtar@...sung.com>,
<linux-samsung-soc@...r.kernel.org>, <kernel@...i.sm>,
<kernel@...gutronix.de>, <festevam@...il.com>,
<linux-media@...r.kernel.org>, <imx@...ts.linux.dev>,
<linux-arm-kernel@...ts.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@...sung.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@...sung.com>
> > +L: linux-arm-kernel@...ts.infradead.org (moderated for non-
> subscribers)
>
> Replace above list with samsung-soc list.
>
I'll change in next patchset.
> > +L: linux-media@...r.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
Powered by blists - more mailing lists