From: Sui Jingfeng <[email protected]>
This make the code in etnaviv_pdev_probe() less twisted, drop the reference
to device node after finished. Also kill a double blank line.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 7d0eeab3e8b7..3446f8eabf59 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -27,6 +27,19 @@
* DRM operations:
*/
+/* If the DT contains at least one available GPU, return a pointer to it */
+
+static struct device_node *etnaviv_of_first_node(void)
+{
+ struct device_node *np;
+
+ for_each_compatible_node(np, NULL, "vivante,gc") {
+ if (of_device_is_available(np))
+ return np;
+ }
+
+ return NULL;
+}
static void load_gpu(struct drm_device *dev)
{
@@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
static int etnaviv_pdev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *first_node = NULL;
+ struct device_node *first_node;
struct component_match *match = NULL;
if (!dev->platform_data) {
@@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
if (!of_device_is_available(core_node))
continue;
- if (!first_node)
- first_node = core_node;
-
drm_of_component_match_add(&pdev->dev, &match,
component_compare_of, core_node);
+
+ of_node_put(core_node);
}
} else {
char **names = dev->platform_data;
@@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
* device as the GPU we found. This assumes that all Vivante
* GPUs in the system share the same DMA constraints.
*/
- if (first_node)
+ first_node = etnaviv_of_first_node();
+ if (first_node) {
of_dma_configure(&pdev->dev, first_node, true);
+ of_node_put(first_node);
+ }
return component_master_add_with_match(dev, &etnaviv_master_ops, match);
}
@@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
* If the DT contains at least one available GPU device, instantiate
* the DRM platform device.
*/
- for_each_compatible_node(np, NULL, "vivante,gc") {
- if (!of_device_is_available(np))
- continue;
+ np = etnaviv_of_first_node();
+ if (np) {
of_node_put(np);
ret = etnaviv_create_platform_device("etnaviv",
&etnaviv_platform_device);
if (ret)
goto unregister_platform_driver;
-
- break;
}
return 0;
--
2.25.1
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <[email protected]>
>
> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
> to device node after finished. Also kill a double blank line.
>
I can't spot the double blank line you claim to remove.
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 7d0eeab3e8b7..3446f8eabf59 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -27,6 +27,19 @@
> * DRM operations:
> */
>
> +/* If the DT contains at least one available GPU, return a pointer to it */
> +
I think the code in the function is simple enough that we don't need a
comment explaining what it does.
Regards,
Lucas
> +static struct device_node *etnaviv_of_first_node(void)
> +{
> + struct device_node *np;
> +
> + for_each_compatible_node(np, NULL, "vivante,gc") {
> + if (of_device_is_available(np))
> + return np;
> + }
> +
> + return NULL;
> +}
>
> static void load_gpu(struct drm_device *dev)
> {
> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
> static int etnaviv_pdev_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct device_node *first_node = NULL;
> + struct device_node *first_node;
> struct component_match *match = NULL;
>
> if (!dev->platform_data) {
> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
> if (!of_device_is_available(core_node))
> continue;
>
> - if (!first_node)
> - first_node = core_node;
> -
> drm_of_component_match_add(&pdev->dev, &match,
> component_compare_of, core_node);
> +
> + of_node_put(core_node);
> }
> } else {
> char **names = dev->platform_data;
> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
> * device as the GPU we found. This assumes that all Vivante
> * GPUs in the system share the same DMA constraints.
> */
> - if (first_node)
> + first_node = etnaviv_of_first_node();
> + if (first_node) {
> of_dma_configure(&pdev->dev, first_node, true);
> + of_node_put(first_node);
> + }
>
> return component_master_add_with_match(dev, &etnaviv_master_ops, match);
> }
> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
> * If the DT contains at least one available GPU device, instantiate
> * the DRM platform device.
> */
> - for_each_compatible_node(np, NULL, "vivante,gc") {
> - if (!of_device_is_available(np))
> - continue;
> + np = etnaviv_of_first_node();
> + if (np) {
> of_node_put(np);
>
> ret = etnaviv_create_platform_device("etnaviv",
> &etnaviv_platform_device);
> if (ret)
> goto unregister_platform_driver;
> -
> - break;
> }
>
> return 0;
Hi,
On 2023/7/17 18:07, Lucas Stach wrote:
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <[email protected]>
>>
>> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
>> to device node after finished. Also kill a double blank line.
>>
> I can't spot the double blank line you claim to remove.
>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 7d0eeab3e8b7..3446f8eabf59 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -27,6 +27,19 @@
>> * DRM operations:
>> */
>>
>> +/* If the DT contains at least one available GPU, return a pointer to it */
>> +
Here is the double blank line my patch remove, it (a blank line) is occupied by
the comment of "/* If the DT contains at least one available GPU, return a pointer to it */"
> I think the code in the function is simple enough that we don't need a
> comment explaining what it does.
Ok, then I'll remove the comment at the next version. Thanks
> Regards,
> Lucas
>
>> +static struct device_node *etnaviv_of_first_node(void)
>> +{
>> + struct device_node *np;
>> +
>> + for_each_compatible_node(np, NULL, "vivante,gc") {
>> + if (of_device_is_available(np))
>> + return np;
>> + }
>> +
>> + return NULL;
>> +}
>>
>> static void load_gpu(struct drm_device *dev)
>> {
>> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
>> static int etnaviv_pdev_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> - struct device_node *first_node = NULL;
>> + struct device_node *first_node;
>> struct component_match *match = NULL;
>>
>> if (!dev->platform_data) {
>> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>> if (!of_device_is_available(core_node))
>> continue;
>>
>> - if (!first_node)
>> - first_node = core_node;
>> -
>> drm_of_component_match_add(&pdev->dev, &match,
>> component_compare_of, core_node);
>> +
>> + of_node_put(core_node);
>> }
>> } else {
>> char **names = dev->platform_data;
>> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>> * device as the GPU we found. This assumes that all Vivante
>> * GPUs in the system share the same DMA constraints.
>> */
>> - if (first_node)
>> + first_node = etnaviv_of_first_node();
>> + if (first_node) {
>> of_dma_configure(&pdev->dev, first_node, true);
>> + of_node_put(first_node);
>> + }
>>
>> return component_master_add_with_match(dev, &etnaviv_master_ops, match);
>> }
>> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
>> * If the DT contains at least one available GPU device, instantiate
>> * the DRM platform device.
>> */
>> - for_each_compatible_node(np, NULL, "vivante,gc") {
>> - if (!of_device_is_available(np))
>> - continue;
>> + np = etnaviv_of_first_node();
>> + if (np) {
>> of_node_put(np);
>>
>> ret = etnaviv_create_platform_device("etnaviv",
>> &etnaviv_platform_device);
>> if (ret)
>> goto unregister_platform_driver;
>> -
>> - break;
>> }
>>
>> return 0;
Hi,
On 2023/7/17 18:07, Lucas Stach wrote:
> Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <[email protected]>
>>
>> This make the code in etnaviv_pdev_probe() less twisted, drop the reference
>> to device node after finished. Also kill a double blank line.
>>
> I can't spot the double blank line you claim to remove.
>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 7d0eeab3e8b7..3446f8eabf59 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -27,6 +27,19 @@
>> * DRM operations:
>> */
>>
>> +/* If the DT contains at least one available GPU, return a pointer to it */
>> +
> I think the code in the function is simple enough that we don't need a
> comment explaining what it does.
Because the DT could disable GPU cores by add "status=disabled" property.
So, only the word *available* in this comments is deserved.
But I'm fine to delete the comment for this function, will be fixed at
the next version.
Thanks for reviewing.
> Regards,
> Lucas
>
>> +static struct device_node *etnaviv_of_first_node(void)
>> +{
>> + struct device_node *np;
>> +
>> + for_each_compatible_node(np, NULL, "vivante,gc") {
>> + if (of_device_is_available(np))
>> + return np;
>> + }
>> +
>> + return NULL;
>> +}
>>
>> static void load_gpu(struct drm_device *dev)
>> {
>> @@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops = {
>> static int etnaviv_pdev_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> - struct device_node *first_node = NULL;
>> + struct device_node *first_node;
>> struct component_match *match = NULL;
>>
>> if (!dev->platform_data) {
>> @@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>> if (!of_device_is_available(core_node))
>> continue;
>>
>> - if (!first_node)
>> - first_node = core_node;
>> -
>> drm_of_component_match_add(&pdev->dev, &match,
>> component_compare_of, core_node);
>> +
>> + of_node_put(core_node);
>> }
>> } else {
>> char **names = dev->platform_data;
>> @@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
>> * device as the GPU we found. This assumes that all Vivante
>> * GPUs in the system share the same DMA constraints.
>> */
>> - if (first_node)
>> + first_node = etnaviv_of_first_node();
>> + if (first_node) {
>> of_dma_configure(&pdev->dev, first_node, true);
>> + of_node_put(first_node);
>> + }
>>
>> return component_master_add_with_match(dev, &etnaviv_master_ops, match);
>> }
>> @@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
>> * If the DT contains at least one available GPU device, instantiate
>> * the DRM platform device.
>> */
>> - for_each_compatible_node(np, NULL, "vivante,gc") {
>> - if (!of_device_is_available(np))
>> - continue;
>> + np = etnaviv_of_first_node();
>> + if (np) {
>> of_node_put(np);
>>
>> ret = etnaviv_create_platform_device("etnaviv",
>> &etnaviv_platform_device);
>> if (ret)
>> goto unregister_platform_driver;
>> -
>> - break;
>> }
>>
>> return 0;