Messages in this thread | | | Date | Thu, 24 Aug 2023 20:31:44 +0200 | Subject | Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core driver | From | Christophe JAILLET <> |
| |
Le 24/08/2023 à 10:01, Jack Zhu a écrit : > Add core driver for StarFive Camera Subsystem. The code parses > the device platform resources and registers related devices. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com> > ---
...
> diff --git a/drivers/staging/media/starfive/camss/Kconfig b/drivers/staging/media/starfive/camss/Kconfig > new file mode 100644 > index 000000000000..8d20e2bd2559 > --- /dev/null > +++ b/drivers/staging/media/starfive/camss/Kconfig > @@ -0,0 +1,17 @@ > +# 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 > + depends on PM > + 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.
stf_camss? (s/-/_)
> diff --git a/drivers/staging/media/starfive/camss/Makefile b/drivers/staging/media/starfive/camss/Makefile > new file mode 100644 > index 000000000000..f53c5cbe958f > --- /dev/null > +++ b/drivers/staging/media/starfive/camss/Makefile > @@ -0,0 +1,9 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for StarFive Camera Subsystem driver > +# > + > +starfive-camss-objs += \ > + stf_camss.o > + > +obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o
I'm not an expert in Makefile files, but this stf_camss.o and starfive-camss.o look strange to me.
> diff --git a/drivers/staging/media/starfive/camss/stf_camss.c b/drivers/staging/media/starfive/camss/stf_camss.c > new file mode 100644 > index 000000000000..75ebc3a35218 > --- /dev/null > +++ b/drivers/staging/media/starfive/camss/stf_camss.c
...
> +static int stfcamss_of_parse_ports(struct stfcamss *stfcamss) > +{ > + struct device_node *node = NULL; > + int ret, num_subdevs = 0; > + > + for_each_endpoint_of_node(stfcamss->dev->of_node, node) { > + struct stfcamss_async_subdev *csd; > + > + if (!of_device_is_available(node)) > + continue; > + > + csd = v4l2_async_nf_add_fwnode_remote(&stfcamss->notifier, > + of_fwnode_handle(node), > + struct stfcamss_async_subdev); > + if (IS_ERR(csd)) { > + ret = PTR_ERR(csd); > + dev_err(stfcamss->dev, "failed to add async notifier\n"); > + v4l2_async_nf_cleanup(&stfcamss->notifier);
having it here, looks strange to me. It is already called in the error handling path of the probe.
Should there be a "of_node_put(node);" if we return here?
> + return ret; > + } > + > + ret = stfcamss_of_parse_endpoint_node(stfcamss, node, csd); > + if (ret) > + return ret; > + > + num_subdevs++; > + } > + > + return num_subdevs; > +}
...
> +static int stfcamss_remove(struct platform_device *pdev) > +{ > + struct stfcamss *stfcamss = platform_get_drvdata(pdev); > + > + v4l2_device_unregister(&stfcamss->v4l2_dev); > + media_device_cleanup(&stfcamss->media_dev);
Is a "v4l2_async_nf_cleanup(&stfcamss->notifier);" missing to match the error handling path of the probe?
> + pm_runtime_disable(&pdev->dev); > + > + return 0; > +} > +
...
| |