Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core driver

[Christophe JAILLET]

Le 25/08/2023 à 12:44, Jack Zhu a écrit :
> Hi Christophe,
>
> Thank you for your comment!
>
> On 2023/8/25 2:31, Christophe JAILLET wrote:
> > 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@xxxxxxxxxx>
> > > Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
> > > ---
> >
> > ...
> >
> > > 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 msstf-cas.
> >
> > stf_camss? (s/-/_)
>
> Refer to the writing method of other media drivers, most of them use hyphen.

Forget about my comment. I have been puzzled by "msstf-cas" here, vs 
"stf_camss" below.

> it may be better to use ‘starfive-camss'？

Yes, I think so.

> > > 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.
>
> Is it better to replace 'stf_camss.o' with 'stf-camss.o', which is consistent
> with the driving style of other media drivers?

No strong opinion on it.
Consistency is always good.

> > > 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?
>
> We do not call a 'get' interface, is it necessary to use the 'put' interface?

for_each_endpoint_of_node() does.

See [1] for doc, and [2] for an example.

[1]: 
https://elixir.bootlin.com/linux/v6.5-rc7/source/include/linux/of_graph.h#L30

[2]: 
https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/gpu/drm/bridge/tc358767.c#L2196



Also, at least because of the recent b8ec754ae4c5 in -next, your patch 
does not compile as-is on -next.

CJ

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