| Message ID | 20211117145829.204360-2-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/simpledrm: Apple M1 / DT platform support fixes | expand |
Hi Am 17.11.21 um 15:58 schrieb Hector Martin: > This matches the simplefb behavior; these nodes are not matched by the > standard OF machinery. This fixes a regression when simpledrm replaces > simeplefb. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/gpu/drm/tiny/simpledrm.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c > index 481b48bde047..2c84f2ea1fa2 100644 > --- a/drivers/gpu/drm/tiny/simpledrm.c > +++ b/drivers/gpu/drm/tiny/simpledrm.c > @@ -2,6 +2,7 @@ > > #include <linux/clk.h> > #include <linux/of_clk.h> > +#include <linux/of_platform.h> > #include <linux/platform_data/simplefb.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = { > > module_platform_driver(simpledrm_platform_driver); > > +static int __init simpledrm_init(void) > +{ > + struct device_node *np; > + > + if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { > + for_each_child_of_node(of_chosen, np) { > + if (of_device_is_compatible(np, "simple-framebuffer")) > + of_platform_device_create(np, NULL, NULL); > + } > + } > + > + return 0; > +} > + > +fs_initcall(simpledrm_init); > + Simpledrm is just a driver, but this is platform setup code. Why is this code located here and not under arch/ or drivers/firmware/? I know that other drivers do similar things, it doesn't seem to belong here. Best regards Thomas > MODULE_DESCRIPTION(DRIVER_DESC); > MODULE_LICENSE("GPL v2"); >
On 18/11/2021 18.19, Thomas Zimmermann wrote: > Hi > > Am 17.11.21 um 15:58 schrieb Hector Martin: >> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = { >> >> module_platform_driver(simpledrm_platform_driver); >> >> +static int __init simpledrm_init(void) >> +{ >> + struct device_node *np; >> + >> + if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { >> + for_each_child_of_node(of_chosen, np) { >> + if (of_device_is_compatible(np, "simple-framebuffer")) >> + of_platform_device_create(np, NULL, NULL); >> + } >> + } >> + >> + return 0; >> +} >> + >> +fs_initcall(simpledrm_init); >> + > > Simpledrm is just a driver, but this is platform setup code. Why is this > code located here and not under arch/ or drivers/firmware/? > > I know that other drivers do similar things, it doesn't seem to belong here. This definitely doesn't belong in either of those, since it is not arch- or firmware-specific. It is implementing support for the standard simple-framebuffer OF binding, which specifies that it must be located within the /chosen node (and thus the default OF setup code won't do the matching for you); this applies to all OF platforms [1] Adding Rob; do you think this should move from simplefb/simpledrm to common OF code? (where?) [1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml
Hi Am 20.11.21 um 04:23 schrieb Hector Martin: > On 18/11/2021 18.19, Thomas Zimmermann wrote: >> Hi >> >> Am 17.11.21 um 15:58 schrieb Hector Martin: >>> @@ -897,5 +898,21 @@ static struct platform_driver >>> simpledrm_platform_driver = { >>> module_platform_driver(simpledrm_platform_driver); >>> +static int __init simpledrm_init(void) >>> +{ >>> + struct device_node *np; >>> + >>> + if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { >>> + for_each_child_of_node(of_chosen, np) { >>> + if (of_device_is_compatible(np, "simple-framebuffer")) >>> + of_platform_device_create(np, NULL, NULL); >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +fs_initcall(simpledrm_init); >>> + >> >> Simpledrm is just a driver, but this is platform setup code. Why is this >> code located here and not under arch/ or drivers/firmware/? >> >> I know that other drivers do similar things, it doesn't seem to belong >> here. > > This definitely doesn't belong in either of those, since it is not arch- > or firmware-specific. It is implementing support for the standard > simple-framebuffer OF binding, which specifies that it must be located > within the /chosen node (and thus the default OF setup code won't do the > matching for you); this applies to all OF platforms [1] > > Adding Rob; do you think this should move from simplefb/simpledrm to > common OF code? (where?) ping! > > [1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml >
On Fri, Nov 19, 2021 at 9:24 PM Hector Martin <marcan@marcan.st> wrote: > > On 18/11/2021 18.19, Thomas Zimmermann wrote: > > Hi > > > > Am 17.11.21 um 15:58 schrieb Hector Martin: > >> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = { > >> > >> module_platform_driver(simpledrm_platform_driver); > >> > >> +static int __init simpledrm_init(void) > >> +{ > >> + struct device_node *np; > >> + > >> + if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { > >> + for_each_child_of_node(of_chosen, np) { > >> + if (of_device_is_compatible(np, "simple-framebuffer")) > >> + of_platform_device_create(np, NULL, NULL); > >> + } > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +fs_initcall(simpledrm_init); > >> + > > > > Simpledrm is just a driver, but this is platform setup code. Why is this > > code located here and not under arch/ or drivers/firmware/? > > > > I know that other drivers do similar things, it doesn't seem to belong here. > > This definitely doesn't belong in either of those, since it is not arch- > or firmware-specific. It is implementing support for the standard > simple-framebuffer OF binding, which specifies that it must be located > within the /chosen node (and thus the default OF setup code won't do the > matching for you); this applies to all OF platforms [1] > > Adding Rob; do you think this should move from simplefb/simpledrm to > common OF code? (where?) of_platform_default_populate_init() should work.
> > > > > > Simpledrm is just a driver, but this is platform setup code. Why is this > > > code located here and not under arch/ or drivers/firmware/? > > > Agreed. Creating platform devices is something for platform code and not really a DRM driver. > > > I know that other drivers do similar things, it doesn't seem to belong here. > > Yeah, the simplefb driver does this but that seems like something that should be changed. > > This definitely doesn't belong in either of those, since it is not arch- > > or firmware-specific. It is implementing support for the standard > > simple-framebuffer OF binding, which specifies that it must be located > > within the /chosen node (and thus the default OF setup code won't do the > > matching for you); this applies to all OF platforms [1] > > > > Adding Rob; do you think this should move from simplefb/simpledrm to > > common OF code? (where?) > > of_platform_default_populate_init() should work. That should work but I still wonder if it is the correct place to add this logic. I think that instead it could be done in the sysfb_create_simplefb() function [0], which already creates the "simple-framebuffer" device for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do the same for OF. That way the simplefb platform device registration code could also be dropped from the driver and users would just need to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same. I have a couple of boards with a bootloader that populates a "simple-framebuffer" in the /chosen node so I could attempt to write the patches. But probably won't happen until next week. [0]: https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/firmware/sysfb_simplefb.c#L60 Best regards, Javier
Hi Am 30.11.21 um 07:44 schrieb Javier Martinez Canillas: >>>> >>>> Simpledrm is just a driver, but this is platform setup code. Why is this >>>> code located here and not under arch/ or drivers/firmware/? >>>> > > Agreed. Creating platform devices is something for platform code and > not really a DRM driver. > >>>> I know that other drivers do similar things, it doesn't seem to belong here. >>> > > Yeah, the simplefb driver does this but that seems like something that > should be changed. > >>> This definitely doesn't belong in either of those, since it is not arch- >>> or firmware-specific. It is implementing support for the standard >>> simple-framebuffer OF binding, which specifies that it must be located >>> within the /chosen node (and thus the default OF setup code won't do the >>> matching for you); this applies to all OF platforms [1] >>> >>> Adding Rob; do you think this should move from simplefb/simpledrm to >>> common OF code? (where?) >> >> of_platform_default_populate_init() should work. > > That should work but I still wonder if it is the correct place to add > this logic. > > I think that instead it could be done in the sysfb_create_simplefb() > function [0], which already creates the "simple-framebuffer" device > for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do > the same for OF. That way the simplefb platform device registration > code could also be dropped from the driver and users would just need > to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same. > > I have a couple of boards with a bootloader that populates a > "simple-framebuffer" in the /chosen node so I could attempt to write > the patches. But probably won't happen until next week. IMHO it's better to keep the OF-related setup in the OF code. The sysfb code is for screen_info. We can try to find common code for OF and sysfb that then lives in a shared location. Using a single global screen_info variable is somewhat awkward these days. In the long term, I can think of pushing sysfb code into architectures. Each architecture would then setup the platform devices that it supports. But that's not really important right now. Best regards Thomas > > [0]: https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/firmware/sysfb_simplefb.c#L60 > > Best regards, > Javier >
Hello Thomas, On Tue, Nov 30, 2021 at 9:31 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Am 30.11.21 um 07:44 schrieb Javier Martinez Canillas: [snip] > > > > I think that instead it could be done in the sysfb_create_simplefb() > > function [0], which already creates the "simple-framebuffer" device > > for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do > > the same for OF. That way the simplefb platform device registration > > code could also be dropped from the driver and users would just need > > to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same. > > > > I have a couple of boards with a bootloader that populates a > > "simple-framebuffer" in the /chosen node so I could attempt to write > > the patches. But probably won't happen until next week. > > IMHO it's better to keep the OF-related setup in the OF code. The sysfb > code is for screen_info. We can try to find common code for OF and sysfb > that then lives in a shared location. > Ok. As long as we don't end with code duplication then that works for me too. > Using a single global screen_info variable is somewhat awkward these > days. In the long term, I can think of pushing sysfb code into > architectures. Each architecture would then setup the platform devices > that it supports. But that's not really important right now. > That makes sense. And provide a set of helpers as you mentioned that could be shared across the different architectures and firmware interfaces. Best regards, Javier
On Tue, Nov 30, 2021 at 12:45 AM Javier Martinez Canillas <javier@dowhile0.org> wrote: > > > > > > > > > Simpledrm is just a driver, but this is platform setup code. Why is this > > > > code located here and not under arch/ or drivers/firmware/? > > > > > > Agreed. Creating platform devices is something for platform code and > not really a DRM driver. > > > > > I know that other drivers do similar things, it doesn't seem to belong here. > > > > > Yeah, the simplefb driver does this but that seems like something that > should be changed. > > > > This definitely doesn't belong in either of those, since it is not arch- > > > or firmware-specific. It is implementing support for the standard > > > simple-framebuffer OF binding, which specifies that it must be located > > > within the /chosen node (and thus the default OF setup code won't do the > > > matching for you); this applies to all OF platforms [1] > > > > > > Adding Rob; do you think this should move from simplefb/simpledrm to > > > common OF code? (where?) > > > > of_platform_default_populate_init() should work. > > That should work but I still wonder if it is the correct place to add > this logic. It is because that is where most of the other devices are created unless the bus handles it. > I think that instead it could be done in the sysfb_create_simplefb() > function [0], which already creates the "simple-framebuffer" device > for x86 legacy BIOS and x86/arm64/riscv EFI so it makes sense to do > the same for OF. That way the simplefb platform device registration > code could also be dropped from the driver and users would just need > to enable CONFIG_SYSFB and CONFIG_SYSFB_SIMPLEFB to have the same. Doesn't look like that would share anything with anything else (BIOS/EFI/ACPI). Rob
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 481b48bde047..2c84f2ea1fa2 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -2,6 +2,7 @@ #include <linux/clk.h> #include <linux/of_clk.h> +#include <linux/of_platform.h> #include <linux/platform_data/simplefb.h> #include <linux/platform_device.h> #include <linux/regulator/consumer.h> @@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = { module_platform_driver(simpledrm_platform_driver); +static int __init simpledrm_init(void) +{ + struct device_node *np; + + if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { + for_each_child_of_node(of_chosen, np) { + if (of_device_is_compatible(np, "simple-framebuffer")) + of_platform_device_create(np, NULL, NULL); + } + } + + return 0; +} + +fs_initcall(simpledrm_init); + MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL v2");
This matches the simplefb behavior; these nodes are not matched by the standard OF machinery. This fixes a regression when simpledrm replaces simeplefb. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/gpu/drm/tiny/simpledrm.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)