| 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: <00e401dc1425$65ea9490$31bfbdb0$@samsung.com>
Date: Sat, 23 Aug 2025 17:29:32 +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
> -----Original Message-----
> From: Inbaraj E <inbaraj.e@...sung.com>
> Sent: 23 August 2025 17:20
> To: 'Krzysztof Kozlowski' <krzk@...nel.org>; 'mturquette@...libre.com'
> <mturquette@...libre.com>; 'sboyd@...nel.org' <sboyd@...nel.org>;
> 'robh@...nel.org' <robh@...nel.org>; 'krzk+dt@...nel.org'
> <krzk+dt@...nel.org>; 'conor+dt@...nel.org' <conor+dt@...nel.org>;
> 's.nawrocki@...sung.com' <s.nawrocki@...sung.com>;
> 's.hauer@...gutronix.de' <s.hauer@...gutronix.de>;
> 'shawnguo@...nel.org' <shawnguo@...nel.org>;
> 'cw00.choi@...sung.com' <cw00.choi@...sung.com>; 'rmfrfs@...il.com'
> <rmfrfs@...il.com>; 'laurent.pinchart@...asonboard.com'
> <laurent.pinchart@...asonboard.com>; 'martink@...teo.de'
> <martink@...teo.de>; 'mchehab@...nel.org' <mchehab@...nel.org>;
> 'linux-fsd@...la.com' <linux-fsd@...la.com>; 'will@...nel.org'
> <will@...nel.org>; 'catalin.marinas@....com' <catalin.marinas@....com>;
> 'pankaj.dubey@...sung.com' <pankaj.dubey@...sung.com>;
> 'shradha.t@...sung.com' <shradha.t@...sung.com>;
> 'ravi.patel@...sung.com' <ravi.patel@...sung.com>
> Cc: 'linux-clk@...r.kernel.org' <linux-clk@...r.kernel.org>;
> 'devicetree@...r.kernel.org' <devicetree@...r.kernel.org>; 'linux-
> kernel@...r.kernel.org' <linux-kernel@...r.kernel.org>;
> 'alim.akhtar@...sung.com' <alim.akhtar@...sung.com>; 'linux-samsung-
> soc@...r.kernel.org' <linux-samsung-soc@...r.kernel.org>;
> 'kernel@...i.sm' <kernel@...i.sm>; 'kernel@...gutronix.de'
> <kernel@...gutronix.de>; 'festevam@...il.com' <festevam@...il.com>;
> 'linux-media@...r.kernel.org' <linux-media@...r.kernel.org>;
> 'imx@...ts.linux.dev' <imx@...ts.linux.dev>; 'linux-arm-
> kernel@...ts.infradead.org' <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?
>
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
Powered by blists - more mailing lists