On 2023/8/2 2:45, Laurent Pinchart wrote:
> Hi Jack,
>
> On Tue, Aug 01, 2023 at 11:24:22AM +0800, Jack Zhu wrote:
>> On 2023/7/27 19:33, Laurent Pinchart wrote:
>> > On Mon, Jun 19, 2023 at 07:28:35PM +0800, Jack Zhu wrote:
>> >> Add basic platform driver for StarFive Camera Subsystem.
>> >>
>> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>> >> Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
>> >> ---
>> >> MAINTAINERS | 1 +
>> >> drivers/media/platform/Kconfig | 1 +
>> >> drivers/media/platform/Makefile | 1 +
>> >> drivers/media/platform/starfive/Kconfig | 5 +
>> >> drivers/media/platform/starfive/Makefile | 2 +
>> >> drivers/media/platform/starfive/camss/Kconfig | 16 +
>> >> .../media/platform/starfive/camss/Makefile | 8 +
>> >> .../media/platform/starfive/camss/stf_camss.c | 338 ++++++++++++++++++
>> >> .../media/platform/starfive/camss/stf_camss.h | 146 ++++++++
>> >> 9 files changed, 518 insertions(+)
>> >> create mode 100644 drivers/media/platform/starfive/Kconfig
>> >> create mode 100644 drivers/media/platform/starfive/Makefile
>> >> create mode 100644 drivers/media/platform/starfive/camss/Kconfig
>> >> create mode 100644 drivers/media/platform/starfive/camss/Makefile
>> >> create mode 100644 drivers/media/platform/starfive/camss/stf_camss.c
>> >> create mode 100644 drivers/media/platform/starfive/camss/stf_camss.h
>
> [snip]
>
>> >> diff --git a/drivers/media/platform/starfive/camss/Kconfig b/drivers/media/platform/starfive/camss/Kconfig
>> >> new file mode 100644
>> >> index 000000000000..dafe1d24324b
>> >> --- /dev/null
>> >> +++ b/drivers/media/platform/starfive/camss/Kconfig
>> >> @@ -0,0 +1,16 @@
>> >> +# SPDX-License-Identifier: GPL-2.0-only
>> >> +config VIDEO_STARFIVE_CAMSS
>> >> + tristate "Starfive Camera Subsystem driver"
>> >> + depends on V4L_PLATFORM_DRIVERS
>> >> + depends on VIDEO_DEV && OF
>> >> + depends on HAS_DMA
>> >
>> > You need to depend on PM, otherwise the runtime PM operations will be
>> > no-ops and the driver won't work as clocks won't be enabled.
>>
>> OK, I will add dependency.
>
> By the way, if it makes it easier for you, you don't need to acknowledge
> every single review comment. You can reply to comments you disagree
> with, or comments that you find unclear. Anything that you agree with
> and will address in the next version can be left unanswered in your
> e-mail replies. It's entirely up to you.
>
Hi Laurent,
Your suggestion is very useful for me. Thanks!
>> >> + select MEDIA_CONTROLLER
>> >> + select VIDEO_V4L2_SUBDEV_API
>> >> + select VIDEOBUF2_DMA_CONTIG
>> >> + select V4L2_FWNODE
>> >> + help
>> >> + Enable this to support for the Starfive Camera subsystem
>> >> + found on Starfive JH7110 SoC.
>> >> +
>> >> + To compile this driver as a module, choose M here: the
>> >> + module will be called stf-camss.
>
> [snip]
>
>> >> diff --git a/drivers/media/platform/starfive/camss/stf_camss.c b/drivers/media/platform/starfive/camss/stf_camss.c
>> >> new file mode 100644
>> >> index 000000000000..dc2b5dba7bd4
>> >> --- /dev/null
>> >> +++ b/drivers/media/platform/starfive/camss/stf_camss.c
>> >> @@ -0,0 +1,338 @@
>
> [snip]
>
>> >> +/*
>> >> + * stfcamss_probe - Probe STFCAMSS platform device
>> >> + * @pdev: Pointer to STFCAMSS platform device
>> >> + *
>> >> + * Return 0 on success or a negative error code on failure
>> >> + */
>> >> +static int stfcamss_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct stfcamss *stfcamss;
>> >> + struct device *dev = &pdev->dev;
>> >> + int ret, num_subdevs;
>> >> + unsigned int i;
>> >> +
>> >> + stfcamss = devm_kzalloc(dev, sizeof(*stfcamss), GFP_KERNEL);
>> >> + if (!stfcamss)
>> >> + return -ENOMEM;
>> >> +
>> >> + for (i = 0; i < ARRAY_SIZE(stfcamss->irq); ++i) {
>> >> + stfcamss->irq[i] = platform_get_irq(pdev, i);
>> >> + if (stfcamss->irq[i] < 0)
>> >> + return dev_err_probe(&pdev->dev, stfcamss->irq[i],
>> >> + "Failed to get irq%d", i);
>> >> + }
>> >> +
>> >> + stfcamss->nclks = ARRAY_SIZE(stfcamss->sys_clk);
>> >> + for (i = 0; i < stfcamss->nclks; ++i)
>> >> + stfcamss->sys_clk[i].id = stfcamss_clocks[i];
>> >> + ret = devm_clk_bulk_get(dev, stfcamss->nclks, stfcamss->sys_clk);
>> >> + if (ret) {
>> >> + dev_err(dev, "Failed to get clk controls\n");
>> >> + return ret;
>> >> + }
>> >> +
>> >> + stfcamss->nrsts = ARRAY_SIZE(stfcamss->sys_rst);
>> >> + for (i = 0; i < stfcamss->nrsts; ++i)
>> >> + stfcamss->sys_rst[i].id = stfcamss_resets[i];
>> >> + ret = devm_reset_control_bulk_get_shared(dev, stfcamss->nrsts,
>> >> + stfcamss->sys_rst);
>> >> + if (ret) {
>> >> + dev_err(dev, "Failed to get reset controls\n");
>> >> + return ret;
>> >> + }
>> >> +
>> >> + ret = stfcamss_get_mem_res(pdev, stfcamss);
>> >> + if (ret) {
>> >> + dev_err(dev, "Could not map registers\n");
>> >> + return ret;
>> >> + }
>> >> +
>> >> + stfcamss->dev = dev;
>> >
>> > Move this right after allocating stfcamss, and drop the pdev argument to
>> > stfcamss_get_mem_res(). The platform device can be retrieved in the
>> > function using to_platform_device().
>>
>> OK, I will modify.
>>
>> >> + platform_set_drvdata(pdev, stfcamss);
>> >> +
>> >> + v4l2_async_nf_init(&stfcamss->notifier);
>> >> +
>> >> + num_subdevs = stfcamss_of_parse_ports(stfcamss);
>> >> + if (num_subdevs < 0) {
>> >> + ret = -ENODEV;
>> >
>> > An error message would be useful, silent errors are hard to debug.
>>
>> OK, will add error printing information.
>>
>> >> + goto err_cleanup_notifier;
>> >> + }
>> >> +
>> >> + stfcamss_mc_init(pdev, stfcamss);
>> >> +
>> >> + ret = v4l2_device_register(stfcamss->dev, &stfcamss->v4l2_dev);
>> >> + if (ret < 0) {
>> >> + dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
>> >> + goto err_cleanup_notifier;
>> >> + }
>> >> +
>> >> + ret = media_device_register(&stfcamss->media_dev);
>> >> + if (ret) {
>> >> + dev_err(dev, "Failed to register media device: %d\n", ret);
>> >> + goto err_unregister_device;
>> >> + }
>> >> +
>> >> + pm_runtime_enable(dev);
>> >
>> > Would it be useful to enable autosuspend too, to avoid expensive
>> > suspend/resume cycles when userspace wants to briefly stop capture and
>> > restart it immediately ?
>>
>> It seems rare to use autosuspend in the Linux camera system.
>
> It's a relatively recent practice, and is more common in sensor drivers
> than ISP drivers, but I think it's a good practice nonetheless. It makes
> stop-reconfigure-start cycles much faster.
>
Yes, I agree with you, but the existing applications on our platform are
relatively simple, and I want to keep this usage for now.
>> >> +
>> >> + stfcamss->notifier.ops = &stfcamss_subdev_notifier_ops;
>> >> + ret = v4l2_async_nf_register(&stfcamss->v4l2_dev, &stfcamss->notifier);
>> >> + if (ret) {
>> >> + dev_err(dev, "Failed to register async subdev nodes: %d\n",
>> >> + ret);
>> >> + goto err_unregister_media_dev;
>> >
>> > You need to disable runtime PM in this error path.
>>
>> OK, will fix it.
>>
>> >> + }
>> >> +
>> >> + return 0;
>> >> +
>> >> +err_unregister_media_dev:
>> >> + media_device_unregister(&stfcamss->media_dev);
>> >> +err_unregister_device:
>> >> + v4l2_device_unregister(&stfcamss->v4l2_dev);
>> >> +err_cleanup_notifier:
>> >> + v4l2_async_nf_cleanup(&stfcamss->notifier);
>> >> + return ret;
>> >> +}
>
> [snip]
>
