Hello, On Wed, Nov 01, 2023 at 06:20:17PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@xxxxxxxxxx> > > The simple-framebuffer device tree bindings document the power-domains > property, so make sure that simplefb supports it. This ensures that the > power domains remain enabled as long as simplefb is active. > > v2: - remove unnecessary call to simplefb_detach_genpds() since that's > already done automatically by devres > - fix crash if power-domains property is missing in DT > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > --- > drivers/video/fbdev/simplefb.c | 93 ++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c > index 18025f34fde7..fe682af63827 100644 > --- a/drivers/video/fbdev/simplefb.c > +++ b/drivers/video/fbdev/simplefb.c > @@ -25,6 +25,7 @@ > #include <linux/of_clk.h> > #include <linux/of_platform.h> > #include <linux/parser.h> > +#include <linux/pm_domain.h> > #include <linux/regulator/consumer.h> > > static const struct fb_fix_screeninfo simplefb_fix = { > @@ -78,6 +79,11 @@ struct simplefb_par { > unsigned int clk_count; > struct clk **clks; > #endif > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS > + unsigned int num_genpds; This is the cause of the crash that occurred on the older patch series. The field is unsigned, a deviation from v6.6:drivers/remoteproc/imx_rproc.c. Instead of making it signed, this version emits an error whenever the count is negative. > + struct device **genpds; > + struct device_link **genpd_links; > +#endif > #if defined CONFIG_OF && defined CONFIG_REGULATOR > bool regulators_enabled; > u32 regulator_count; > @@ -432,6 +438,89 @@ static void simplefb_regulators_enable(struct simplefb_par *par, > static void simplefb_regulators_destroy(struct simplefb_par *par) { } > #endif > > +#if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS > +static void simplefb_detach_genpds(void *res) > +{ > + struct simplefb_par *par = res; > + unsigned int i = par->num_genpds; > + > + if (par->num_genpds <= 1) > + return; > + > + while (i--) { > + if (par->genpd_links[i]) > + device_link_del(par->genpd_links[i]); > + > + if (!IS_ERR_OR_NULL(par->genpds[i])) > + dev_pm_domain_detach(par->genpds[i], true); > + } > +} > + > +static int simplefb_attach_genpds(struct simplefb_par *par, > + struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + unsigned int i; > + int err; > + > + err = of_count_phandle_with_args(dev->of_node, "power-domains", > + "#power-domain-cells"); > + if (err < 0) { > + dev_info(dev, "failed to parse power-domains: %d\n", err); > + return err; This error path is taken when there is no power-domains property in the device tree with err = -ENOENT. Strangely, this does not suppress the error like the next if statement, even though it is possible that nothing is wrong. > + } > + > + par->num_genpds = err; > + > + /* > + * Single power-domain devices are handled by the driver core, so > + * nothing to do here. > + */ > + if (par->num_genpds <= 1) > + return 0; > + > + par->genpds = devm_kcalloc(dev, par->num_genpds, sizeof(*par->genpds), > + GFP_KERNEL); <snip> > @@ -518,6 +607,10 @@ static int simplefb_probe(struct platform_device *pdev) > if (ret < 0) > goto error_clocks; > > + ret = simplefb_attach_genpds(par, pdev); > + if (ret < 0) > + goto error_regulators; With the error case specified above, not specifying power-domains (which is valid according to dtschema) causes the entire driver to fail whenever there are no power domains in the device tree. On google-sargo, this causes a bug where the framebuffer fails to probe: [ 0.409290] simple-framebuffer 9c000000.framebuffer: failed to parse power-domains: -2 [ 0.409340] simple-framebuffer: probe of 9c000000.framebuffer failed with error -2