Re: [PATCH v7 3/6] media: starfive: camss: Add basic driver

[Jack Zhu <jack.zhu@starfivetech.com>]

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@linaro.org>
>> >> Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
>> >> ---
>> >>  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]
>