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
