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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

> >> +	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.

> >> +
> >> +	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]

-- 
Regards,

Laurent Pinchart