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

[Jack Zhu]

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 stf-camss.
> 
> stf_camss? (s/-/_)
> 

Refer to the writing method of other media drivers, most of them use hyphen. It
may be better to use ‘starfive-camss'？

>> 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?

>> 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?

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