Re: [PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node

[Sui Jingfeng <sui.jingfeng@xxxxxxxxx>]

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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> >   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;