| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bff9e368-9c42-144c-bdbc-9b9fcd04ec6b@wanadoo.fr>
Date: Fri, 25 Aug 2023 23:44:12 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Jack Zhu <jack.zhu@...rfivetech.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Robert Foss <rfoss@...nel.org>,
Todor Tomov <todor.too@...il.com>, bryan.odonoghue@...aro.org,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-staging@...ts.linux.dev,
changhuang.liang@...rfivetech.com
Subject: Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core
driver
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@...aro.org>
>>> Signed-off-by: Jack Zhu <jack.zhu@...rfivetech.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 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;
>>> +}
>>> +
>>
>> ...
>
Powered by blists - more mailing lists