On Thu, Oct 26, 2023 at 02:50:27PM +0200, Hans de Goede wrote:
> Hi,
>
> Thank you for your patches.
>
> On 10/11/23 16:38, 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.
> >
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > drivers/video/fbdev/simplefb.c | 93 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 91 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> > index 18025f34fde7..e69fb0ad2d54 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;
> > + 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,83 @@ 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);
> > + }
>
> Using this i-- construct means that genpd at index 0 will
> not be cleaned up.
This is actually a common variant to clean up in reverse order. You'll
find this used a lot in core code and so on. It has the advantage that
you can use it to unwind (not the case here) because i will already be
set to the right value, typically. It's also nice because it works for
unsigned integers.
Note that this uses the postfix decrement, so evaluation happens before
the decrement and therefore the last iteration of the loop will run with
i == 0. For unsigned integers this also means that after the loop the
variable will actually have wrapped around, but that's usually not a
problem since it isn't used after this point anymore.
> > static int simplefb_probe(struct platform_device *pdev)
> > {
> > int ret;
> > @@ -518,6 +601,10 @@ static int simplefb_probe(struct platform_device *pdev)
> > if (ret < 0)
> > goto error_clocks;
> >
> > + ret = simplefb_attach_genpd(par, pdev);
> > + if (ret < 0)
> > + goto error_regulators;
> > +
> > simplefb_clocks_enable(par, pdev);
> > simplefb_regulators_enable(par, pdev);
> >
> > @@ -534,18 +621,20 @@ static int simplefb_probe(struct platform_device *pdev)
> > ret = devm_aperture_acquire_for_platform_device(pdev, par->base, par->size);
> > if (ret) {
> > dev_err(&pdev->dev, "Unable to acquire aperture: %d\n", ret);
> > - goto error_regulators;
> > + goto error_genpds;
>
> This is not necessary since simplefb_attach_genpd() ends with:
>
> devm_add_action_or_reset(dev, simplefb_detach_genpds, par)
>
> Which causes simplefb_detach_genpds() to run when probe() fails.
Yes, you're right. I've removed all these explicit cleanup paths since
they are not needed.
>
> > }
> > ret = register_framebuffer(info);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
> > - goto error_regulators;
> > + goto error_genpds;
> > }
> >
> > dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
> >
> > return 0;
> >
> > +error_genpds:
> > + simplefb_detach_genpds(par);
>
> As the kernel test robot (LKP) already pointed out this is causing
> compile errors when #if defined CONFIG_OF && defined CONFIG_PM_GENERIC_DOMAINS
> evaluates as false.
>
> Since there is no simplefb_detach_genpds() stub in the #else, but as
> mentioned above this is not necessary so just remove it.
Yep, done.
Thanks,
Thierry
Attachment:
signature.asc
Description: PGP signature
