| Message ID | 20191218191017.2895-5-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote: > regmap has special API to enable the controller bus clock while > initializing register space, and current driver is using > devm_regmap_init_mmio_clk which require to specify bus > clk_id argument as "bus" > > But, the usage of clocks are varies between different Allwinner > DSI controllers. Clocking in A33 would need bus and mod clocks > where as A64 would need only bus clock. > > Since A64 support only single bus clock, it is optional to > specify the clock-names on the controller device tree node. > So using NULL on clk_id would get the attached clock. > > To support clk_id as "bus" and "NULL" during clock enablement > between controllers, this patch add generic code to handle > the bus clock using regmap_mmio_attach_clk with associated > regmap APIs. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v13: > - update the changes since has_mod_clk is dropped in previous patch > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++----- > 1 file changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > index 68b88a3dc4c5..de8955fbeb00 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = { > static int sun6i_dsi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + const char *bus_clk_name = NULL; > struct sun6i_dsi *dsi; > struct resource *res; > void __iomem *base; > @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > dsi->host.ops = &sun6i_dsi_host_ops; > dsi->host.dev = dev; > > + if (of_device_is_compatible(dev->of_node, > + "allwinner,sun6i-a31-mipi-dsi")) > + bus_clk_name = "bus"; > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(dev, res); > if (IS_ERR(base)) { > @@ -1107,25 +1112,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 = devm_regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config); > + 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, bus_clk_name); > + 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 { > + ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk); > + if (ret) > + goto err_bus_clk; > + } > + > if (of_device_is_compatible(dev->of_node, > "allwinner,sun6i-a31-mipi-dsi")) { > 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; > } > } > > @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > pm_runtime_disable(dev); > err_unprotect_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); You still have an unbalanced clk_get / clk_put here > +err_regmap: > + regmap_exit(dsi->regs); That's not needed. > return ret; > } > > @@ -1177,6 +1201,11 @@ static int sun6i_dsi_remove(struct platform_device *pdev) > pm_runtime_disable(dev); > clk_rate_exclusive_put(dsi->mod_clk); > > + if (!IS_ERR(dsi->bus_clk)) > + regmap_mmio_detach_clk(dsi->regs); > + > + regmap_exit(dsi->regs); Same thing here. Maxime
On Thu, Dec 19, 2019 at 3:35 AM Maxime Ripard <mripard@kernel.org> wrote: > > On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote: > > regmap has special API to enable the controller bus clock while > > initializing register space, and current driver is using > > devm_regmap_init_mmio_clk which require to specify bus > > clk_id argument as "bus" > > > > But, the usage of clocks are varies between different Allwinner > > DSI controllers. Clocking in A33 would need bus and mod clocks > > where as A64 would need only bus clock. > > > > Since A64 support only single bus clock, it is optional to > > specify the clock-names on the controller device tree node. > > So using NULL on clk_id would get the attached clock. > > > > To support clk_id as "bus" and "NULL" during clock enablement > > between controllers, this patch add generic code to handle > > the bus clock using regmap_mmio_attach_clk with associated > > regmap APIs. > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes for v13: > > - update the changes since has_mod_clk is dropped in previous patch > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++----- > > 1 file changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > index 68b88a3dc4c5..de8955fbeb00 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = { > > static int sun6i_dsi_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > + const char *bus_clk_name = NULL; > > struct sun6i_dsi *dsi; > > struct resource *res; > > void __iomem *base; > > @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > dsi->host.ops = &sun6i_dsi_host_ops; > > dsi->host.dev = dev; > > > > + if (of_device_is_compatible(dev->of_node, > > + "allwinner,sun6i-a31-mipi-dsi")) > > + bus_clk_name = "bus"; > > + > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > base = devm_ioremap_resource(dev, res); > > if (IS_ERR(base)) { > > @@ -1107,25 +1112,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 = devm_regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config); > > + 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, bus_clk_name); > > + 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 { > > + ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk); > > + if (ret) > > + goto err_bus_clk; > > + } > > + > > if (of_device_is_compatible(dev->of_node, > > "allwinner,sun6i-a31-mipi-dsi")) { > > 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; > > } > > } > > > > @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > pm_runtime_disable(dev); > > err_unprotect_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); > > You still have an unbalanced clk_get / clk_put here You mean it is not needed right since devm_clk_get has release call via devres_alloc? or the wrong position for clk_put? > > > +err_regmap: > > + regmap_exit(dsi->regs); > > That's not needed. Yes. look like __devm_regmap_init has release call with regmap_exit. Thanks for the comments, I will update these and send next version. Let me know if you have any more comments?
On Sat, Dec 21, 2019 at 05:11:00PM +0530, Jagan Teki wrote: > On Thu, Dec 19, 2019 at 3:35 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > On Thu, Dec 19, 2019 at 12:40:14AM +0530, Jagan Teki wrote: > > > regmap has special API to enable the controller bus clock while > > > initializing register space, and current driver is using > > > devm_regmap_init_mmio_clk which require to specify bus > > > clk_id argument as "bus" > > > > > > But, the usage of clocks are varies between different Allwinner > > > DSI controllers. Clocking in A33 would need bus and mod clocks > > > where as A64 would need only bus clock. > > > > > > Since A64 support only single bus clock, it is optional to > > > specify the clock-names on the controller device tree node. > > > So using NULL on clk_id would get the attached clock. > > > > > > To support clk_id as "bus" and "NULL" during clock enablement > > > between controllers, this patch add generic code to handle > > > the bus clock using regmap_mmio_attach_clk with associated > > > regmap APIs. > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > --- > > > Changes for v13: > > > - update the changes since has_mod_clk is dropped in previous patch > > > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++----- > > > 1 file changed, 37 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > index 68b88a3dc4c5..de8955fbeb00 100644 > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > > @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = { > > > static int sun6i_dsi_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > + const char *bus_clk_name = NULL; > > > struct sun6i_dsi *dsi; > > > struct resource *res; > > > void __iomem *base; > > > @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > > dsi->host.ops = &sun6i_dsi_host_ops; > > > dsi->host.dev = dev; > > > > > > + if (of_device_is_compatible(dev->of_node, > > > + "allwinner,sun6i-a31-mipi-dsi")) > > > + bus_clk_name = "bus"; > > > + > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > base = devm_ioremap_resource(dev, res); > > > if (IS_ERR(base)) { > > > @@ -1107,25 +1112,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 = devm_regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config); > > > + 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, bus_clk_name); > > > + 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 { > > > + ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk); > > > + if (ret) > > > + goto err_bus_clk; > > > + } > > > + > > > if (of_device_is_compatible(dev->of_node, > > > "allwinner,sun6i-a31-mipi-dsi")) { > > > 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; > > > } > > > } > > > > > > @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > > pm_runtime_disable(dev); > > > err_unprotect_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); > > > > You still have an unbalanced clk_get / clk_put here > > You mean it is not needed right since devm_clk_get has release call > via devres_alloc? Yes
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 68b88a3dc4c5..de8955fbeb00 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -1081,6 +1081,7 @@ static const struct component_ops sun6i_dsi_ops = { static int sun6i_dsi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + const char *bus_clk_name = NULL; struct sun6i_dsi *dsi; struct resource *res; void __iomem *base; @@ -1094,6 +1095,10 @@ static int sun6i_dsi_probe(struct platform_device *pdev) dsi->host.ops = &sun6i_dsi_host_ops; dsi->host.dev = dev; + if (of_device_is_compatible(dev->of_node, + "allwinner,sun6i-a31-mipi-dsi")) + bus_clk_name = "bus"; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(dev, res); if (IS_ERR(base)) { @@ -1107,25 +1112,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 = devm_regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config); + 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, bus_clk_name); + 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 { + ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk); + if (ret) + goto err_bus_clk; + } + if (of_device_is_compatible(dev->of_node, "allwinner,sun6i-a31-mipi-dsi")) { 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; } } @@ -1164,6 +1180,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev) pm_runtime_disable(dev); err_unprotect_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; } @@ -1177,6 +1201,11 @@ static int sun6i_dsi_remove(struct platform_device *pdev) pm_runtime_disable(dev); clk_rate_exclusive_put(dsi->mod_clk); + if (!IS_ERR(dsi->bus_clk)) + regmap_mmio_detach_clk(dsi->regs); + + regmap_exit(dsi->regs); + return 0; }
regmap has special API to enable the controller bus clock while initializing register space, and current driver is using devm_regmap_init_mmio_clk which require to specify bus clk_id argument as "bus" But, the usage of clocks are varies between different Allwinner DSI controllers. Clocking in A33 would need bus and mod clocks where as A64 would need only bus clock. Since A64 support only single bus clock, it is optional to specify the clock-names on the controller device tree node. So using NULL on clk_id would get the attached clock. To support clk_id as "bus" and "NULL" during clock enablement between controllers, this patch add generic code to handle the bus clock using regmap_mmio_attach_clk with associated regmap APIs. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v13: - update the changes since has_mod_clk is dropped in previous patch drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 45 +++++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-)