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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2582 bytes --]

Hi

Am 13.04.22 um 12:45 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks for working on this.
> 
> On 4/13/22 11:24, Thomas Zimmermann wrote:
>> Create a platform device for each OF-declared framebuffer and have
>> offb bind to these devices. Allows for real hot-unplugging and other
>> drivers besides offb.
>>
>> Originally, offb created framebuffer devices while initializing its
>> module by parsing the OF device tree. No actual Linux device was set
>> up. This tied OF framebuffers to offb and makes writing other drivers
>> for the OF framebuffers complicated. The absence of a Linux device
>> prevented real hot-unplugging. Adding a distinct platform device for
>> each OF framebuffer solves both problems. Specifically, a DRM drivers
>> can now provide graphics output with modern userspace.
>>
>> Some of the offb init code is now located in the OF initialization.
>> There's now also an implementation of of_platform_default_populate_init(),
>> which was missing before. The OF side creates different devices for
>> either OF display nodes or bootx displays as they require different
>> handling by the driver. The offb drivers picks up each type of device
>> and runs the appropriate fbdev initialization.
>>
>> Tested with OF display nodes on qemu's ppc64le target.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [snip]
> 
>> +	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);
> 
> Shouldn't check for the return value here too ?

Failing is probably not useful, as it's not the main FB used for 
booting. Printing an error message wouldn't hurt, I guess. I'll also 
update the new driver registration in offb with an error message.

> 
> Other than this small nit, it looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

Thanks.

Best regards
Thomas

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]