| Message ID | 20230623100822.274706-9-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/etnaviv: Various cleanup | expand |
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng: > From: Sui Jingfeng <suijingfeng@loongson.cn> > > 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 <suijingfeng@loongson.cn> > --- > 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 <suijingfeng@loongson.cn> >> >> 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 <suijingfeng@loongson.cn> >> --- >> 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 <suijingfeng@loongson.cn> >> >> 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 <suijingfeng@loongson.cn> >> --- >> 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;
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;