linux-kernel - Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191128175134.kcgtylfo4ax6ifz7@gilmour.lan>
Date:   Thu, 28 Nov 2019 18:51:34 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Jagan Teki <jagan@...rulasolutions.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        devicetree <devicetree@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        linux-sunxi <linux-sunxi@...glegroups.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Chen-Yu Tsai <wens@...e.org>, Rob Herring <robh+dt@...nel.org>,
        Michael Trimarchi <michael@...rulasolutions.com>,
        linux-amarula <linux-amarula@...rulasolutions.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Icenowy Zheng <icenowy@...c.io>
Subject: Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

On Sat, Nov 23, 2019 at 01:20:21AM +0530, Jagan Teki wrote:
> > > Please have a look at this snippet, I have used your second
> > > suggestions. let me know if you have any comments?
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 8fa90cfc2ac8..91c95e56d870 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >          return PTR_ERR(dsi->regulator);
> > >      }
> > >
> > > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > > -                          &sun6i_dsi_regmap_config);
> > > -    if (IS_ERR(dsi->regs)) {
> > > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > > -        return PTR_ERR(dsi->regs);
> > > -    }
> > > -
> > >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> > >      if (IS_ERR(dsi->reset)) {
> > >          dev_err(dev, "Couldn't get our reset line\n");
> > >          return PTR_ERR(dsi->reset);
> > >      }
> > >
> > > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
> >
> > You should use the devm variant here
>
> Sure.
>
> >
> > > +    if (IS_ERR(dsi->regs)) {
> > > +        dev_err(dev, "Couldn't init regmap\n");
> > > +        return PTR_ERR(dsi->regs);
> > > +    }
> > > +
> > > +    dsi->bus_clk = devm_clk_get(dev, NULL);
> >
> > I guess you still need to pass 'bus' here?
>
> But the idea here is not to specify clock name explicitly to support
> A64. otherwise A64 would fail as we are not specifying the clock-names
> explicitly on dsi node.

Right. But you have no guarantee that the bus clock is going to be the
first one on the other SoCs either.

What about something like that instead:

char *clk_name = NULL;
if (dsi->has_mod_clk)
    clk_name = "bus";

clk = devm_clk_get(dev, clk_name);
if (IS_ERR(clk))
    return PTR_ERR(clk));

regmap_mmio_attach_clk(regmap, clk);

>
> dsi: dsi@...0000 {
>        compatible = "allwinner,sun50i-a64-mipi-dsi";
>        reg = <0x01ca0000 0x1000>;
>        interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>        clocks = <&ccu CLK_BUS_MIPI_DSI>;
>        resets = <&ccu RST_BUS_MIPI_DSI>;
>       phys = <&dphy>;
>       phy-names = "dphy";
> .....
> };
>
> >
> > > +    if (IS_ERR(dsi->bus_clk)) {
> > > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > > +        ret = PTR_ERR(dsi->bus_clk);
> > > +        goto err_regmap;
> > > +    } else {
> > > +        printk("Jagan.. Got the BUS clock\n");
> > > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > +        if (ret)
> > > +            goto err_bus_clk;
> > > +    }
> > > +
> > >      if (dsi->variant->has_mod_clk) {
> > >          dsi->mod_clk = devm_clk_get(dev, "mod");
> > >          if (IS_ERR(dsi->mod_clk)) {
> > >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > > -            return PTR_ERR(dsi->mod_clk);
> > > +            ret = PTR_ERR(dsi->mod_clk);
> > > +            goto err_attach_clk;
> > >          }
> > >      }
> > >
> > > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >  err_unprotect_clk:
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > +err_attach_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +err_bus_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        clk_put(dsi->bus_clk);
> > > +err_regmap:
> > > +    regmap_exit(dsi->regs);
> > >      return ret;
> > >  }
> > >
> > > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > >
> > > +    if (!IS_ERR(dsi->bus_clk)) {
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +        clk_put(dsi->bus_clk);
> >
> > This will trigger a warning, you put down the reference twice
>
> You mean regmap_mmio_detach_clk will put the clk?

No, devm_clk_get will.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)