Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 19.04.22 um 15:30 schrieb Rob Herring:
...
> > -#ifndef CONFIG_PPC
> >   static const struct of_device_id reserved_mem_matches[] = {
> >   	{ .compatible = "qcom,rmtfs-mem" },
> >   	{ .compatible = "qcom,cmd-db" },
> > @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
> >   
> >   static int __init of_platform_default_populate_init(void)
> >   {
> > -	struct device_node *node;
> > -
>
> As both if/else clauses need 'node', I'd keep this declared here.

Ok.

> >   	device_links_supplier_sync_state_pause();
> >   
> >   	if (!of_have_populated_dt())
> >   		return -ENODEV;
> >   
> > -	/*
> > -	 * Handle certain compatibles explicitly, since we don't want to create
> > -	 * platform_devices for every node in /reserved-memory with a
> > -	 * "compatible",
> > -	 */
> > -	for_each_matching_node(node, reserved_mem_matches)
> > -		of_platform_device_create(node, NULL, NULL);
> > +	if (IS_ENABLED(CONFIG_PPC)) {
> > +		struct device_node *boot_display = NULL;
> > +		struct device_node *node;
> > +		struct platform_device *dev;
> > +		int ret;
> > +
> > +		/* Check if we have a MacOS display without a node spec */
> > +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> > +			/*
> > +			 * The old code tried to work out which node was the MacOS
> > +			 * display based on the address. I'm dropping that since the
> > +			 * lack of a node spec only happens with old BootX versions
> > +			 * (users can update) and with this code, they'll still get
> > +			 * a display (just not the palette hacks).
> > +			 */
> > +			dev = platform_device_alloc("bootx-noscreen", 0);
> > +			if (WARN_ON(!dev))
> > +				return -ENOMEM;
> > +			ret = platform_device_add(dev);
> > +			if (WARN_ON(ret)) {
> > +				platform_device_put(dev);
> > +				return ret;
> > +			}
> > +		}
> >   
> > -	node = of_find_node_by_path("/firmware");
> > -	if (node) {
> > -		of_platform_populate(node, NULL, NULL, NULL);
> > -		of_node_put(node);
> > -	}
> > +		/*
> > +		 * For OF framebuffers, first create the device for the boot display,
> > +		 * then for the other framebuffers. Only fail for the boot display;
> > +		 * ignore errors for the rest.
> > +		 */
> > +		for_each_node_by_type(node, "display") {
> > +			if (!of_get_property(node, "linux,opened", NULL) ||
> > +			    !of_get_property(node, "linux,boot-display", NULL))
> > +				continue;
> > +			dev = of_platform_device_create(node, "of-display", NULL);
> > +			if (WARN_ON(!dev))
> > +				return -ENOMEM;
> > +			boot_display = node;
> > +			break;
> > +		}
> > +		for_each_node_by_type(node, "display") {
> > +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> > +				continue;
> > +			of_platform_device_create(node, "of-display", NULL);
> > +		}
> >   
> > -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> > -	of_platform_device_create(node, NULL, NULL);
> > -	of_node_put(node);
> > +	} else {
> > +		struct device_node *node;
> > +
> > +		/*
> > +		 * Handle certain compatibles explicitly, since we don't want to create
> > +		 * platform_devices for every node in /reserved-memory with a
> > +		 * "compatible",
> > +		 */
> > +		for_each_matching_node(node, reserved_mem_matches)
> > +			of_platform_device_create(node, NULL, NULL);
> >   
> > -	/* Populate everything else. */
> > -	of_platform_default_populate(NULL, NULL, NULL);
> > +		node = of_find_node_by_path("/firmware");
> > +		if (node) {
> > +			of_platform_populate(node, NULL, NULL, NULL);
> > +			of_node_put(node);
> > +		}
> > +
> > +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> > +		of_platform_device_create(node, NULL, NULL);
> > +		of_node_put(node);
>
> In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow
> that? Maybe no one cares ATM, but that could change. Either way:

Support for these framebuffers has always been mutually exclusive. The 
offb driver, which originally contained the code, depends on CONFIG_PPC. 
And PPC never supported simple-framebuffer anywhere.

> Reviewed-by: Rob Herring <robh@xxxxxxxxxx>

Thank you.

Best regards
Thomas

> > +
> > +		/* Populate everything else. */
> > +		of_platform_default_populate(NULL, NULL, NULL);
> > +	}
> >   
> >   	return 0;
> >   }
> > @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
> >   	return 0;
> >   }
> >   late_initcall_sync(of_platform_sync_state_init);
> > -#endif
> >   
> >   int of_platform_device_destroy(struct device *dev, void *data)
> >   {

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature