| Message ID | 20191203134816.5319-4-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Dec 3, 2019 at 9:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > As per the user manual, look like mod clock is not mandatory > for all Allwinner MIPI DSI controllers, it is connected to > CLK_DSI_SCLK for A31 and not available in A64. > > So add has_mod_clk quirk and process the mod clk accordingly. > > Tested-by: Merlijn Wajer <merlijn@wizzup.org> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes for v12: > - none > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 38 ++++++++++++++++++-------- > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 5 ++++ > 2 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > index c958ca9bae63..8c4c541224dd 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -11,6 +11,7 @@ > #include <linux/crc-ccitt.h> > #include <linux/module.h> > #include <linux/of_address.h> > +#include <linux/of_device.h> > #include <linux/phy/phy-mipi-dphy.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > @@ -1093,6 +1094,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > dsi->dev = dev; > dsi->host.ops = &sun6i_dsi_host_ops; > dsi->host.dev = dev; > + dsi->variant = of_device_get_match_data(dev); > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > base = devm_ioremap_resource(dev, res); > @@ -1120,17 +1122,20 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > return PTR_ERR(dsi->reset); > } > > - 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); > + 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); > + } > } > > /* > * In order to operate properly, that clock seems to be always > * set to 297MHz. > */ > - clk_set_rate_exclusive(dsi->mod_clk, 297000000); > + if (dsi->variant->has_mod_clk) > + clk_set_rate_exclusive(dsi->mod_clk, 297000000); The clk API can handle NULL pointers, so you don't need to add the if here... > > dsi->dphy = devm_phy_get(dev, "dphy"); > if (IS_ERR(dsi->dphy)) { > @@ -1160,7 +1165,8 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > err_pm_disable: > pm_runtime_disable(dev); > err_unprotect_clk: > - clk_rate_exclusive_put(dsi->mod_clk); > + if (dsi->variant->has_mod_clk) > + clk_rate_exclusive_put(dsi->mod_clk); and here... > return ret; > } > > @@ -1172,7 +1178,8 @@ static int sun6i_dsi_remove(struct platform_device *pdev) > component_del(&pdev->dev, &sun6i_dsi_ops); > mipi_dsi_host_unregister(&dsi->host); > pm_runtime_disable(dev); > - clk_rate_exclusive_put(dsi->mod_clk); > + if (dsi->variant->has_mod_clk) > + clk_rate_exclusive_put(dsi->mod_clk); and here ... > > return 0; > } > @@ -1189,7 +1196,8 @@ static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev) > } > > reset_control_deassert(dsi->reset); > - clk_prepare_enable(dsi->mod_clk); > + if (dsi->variant->has_mod_clk) > + clk_prepare_enable(dsi->mod_clk); and here... > > /* > * Enable the DSI block. > @@ -1217,7 +1225,8 @@ static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev) > { > struct sun6i_dsi *dsi = dev_get_drvdata(dev); > > - clk_disable_unprepare(dsi->mod_clk); > + if (dsi->variant->has_mod_clk) > + clk_disable_unprepare(dsi->mod_clk); and here. > reset_control_assert(dsi->reset); > regulator_disable(dsi->regulator); > > @@ -1230,9 +1239,16 @@ static const struct dev_pm_ops sun6i_dsi_pm_ops = { > NULL) > }; > > +static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi = { > + .has_mod_clk = true, > +}; > + > static const struct of_device_id sun6i_dsi_of_table[] = { > - { .compatible = "allwinner,sun6i-a31-mipi-dsi" }, > - { } > + { > + .compatible = "allwinner,sun6i-a31-mipi-dsi", > + .data = &sun6i_a31_mipi_dsi, > + }, > + { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table); > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > index 3f4846f581ef..d791c9f6fccf 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > @@ -15,6 +15,10 @@ > > #define SUN6I_DSI_TCON_DIV 4 > > +struct sun6i_dsi_variant { > + bool has_mod_clk; > +}; > + You could choose to put this above the probe function, since this isn't used anywhere else, and a pointer field doesn't need the full definition. ChenYu > struct sun6i_dsi { > struct drm_connector connector; > struct drm_encoder encoder; > @@ -31,6 +35,7 @@ struct sun6i_dsi { > struct sun4i_drv *drv; > struct mipi_dsi_device *device; > struct drm_panel *panel; > + const struct sun6i_dsi_variant *variant; > }; > > static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host) > -- > 2.18.0.321.gffc6fa0e3 >
On Tue, Dec 3, 2019 at 8:09 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Tue, Dec 3, 2019 at 9:48 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > As per the user manual, look like mod clock is not mandatory > > for all Allwinner MIPI DSI controllers, it is connected to > > CLK_DSI_SCLK for A31 and not available in A64. > > > > So add has_mod_clk quirk and process the mod clk accordingly. > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes for v12: > > - none > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 38 ++++++++++++++++++-------- > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 5 ++++ > > 2 files changed, 32 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > index c958ca9bae63..8c4c541224dd 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > > @@ -11,6 +11,7 @@ > > #include <linux/crc-ccitt.h> > > #include <linux/module.h> > > #include <linux/of_address.h> > > +#include <linux/of_device.h> > > #include <linux/phy/phy-mipi-dphy.h> > > #include <linux/phy/phy.h> > > #include <linux/platform_device.h> > > @@ -1093,6 +1094,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > dsi->dev = dev; > > dsi->host.ops = &sun6i_dsi_host_ops; > > dsi->host.dev = dev; > > + dsi->variant = of_device_get_match_data(dev); > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > base = devm_ioremap_resource(dev, res); > > @@ -1120,17 +1122,20 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > return PTR_ERR(dsi->reset); > > } > > > > - 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); > > + 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); > > + } > > } > > > > /* > > * In order to operate properly, that clock seems to be always > > * set to 297MHz. > > */ > > - clk_set_rate_exclusive(dsi->mod_clk, 297000000); > > + if (dsi->variant->has_mod_clk) > > + clk_set_rate_exclusive(dsi->mod_clk, 297000000); > > The clk API can handle NULL pointers, so you don't need to add the if here... Ohh. I'm not aware of this. does it added recently? > > > > > dsi->dphy = devm_phy_get(dev, "dphy"); > > if (IS_ERR(dsi->dphy)) { > > @@ -1160,7 +1165,8 @@ static int sun6i_dsi_probe(struct platform_device *pdev) > > err_pm_disable: > > pm_runtime_disable(dev); > > err_unprotect_clk: > > - clk_rate_exclusive_put(dsi->mod_clk); > > + if (dsi->variant->has_mod_clk) > > + clk_rate_exclusive_put(dsi->mod_clk); > > and here... > > > return ret; > > } > > > > @@ -1172,7 +1178,8 @@ static int sun6i_dsi_remove(struct platform_device *pdev) > > component_del(&pdev->dev, &sun6i_dsi_ops); > > mipi_dsi_host_unregister(&dsi->host); > > pm_runtime_disable(dev); > > - clk_rate_exclusive_put(dsi->mod_clk); > > + if (dsi->variant->has_mod_clk) > > + clk_rate_exclusive_put(dsi->mod_clk); > > and here ... > > > > > return 0; > > } > > @@ -1189,7 +1196,8 @@ static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev) > > } > > > > reset_control_deassert(dsi->reset); > > - clk_prepare_enable(dsi->mod_clk); > > + if (dsi->variant->has_mod_clk) > > + clk_prepare_enable(dsi->mod_clk); > > and here... > > > > > /* > > * Enable the DSI block. > > @@ -1217,7 +1225,8 @@ static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev) > > { > > struct sun6i_dsi *dsi = dev_get_drvdata(dev); > > > > - clk_disable_unprepare(dsi->mod_clk); > > + if (dsi->variant->has_mod_clk) > > + clk_disable_unprepare(dsi->mod_clk); > > and here. > > > reset_control_assert(dsi->reset); > > regulator_disable(dsi->regulator); > > > > @@ -1230,9 +1239,16 @@ static const struct dev_pm_ops sun6i_dsi_pm_ops = { > > NULL) > > }; > > > > +static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi = { > > + .has_mod_clk = true, > > +}; > > + > > static const struct of_device_id sun6i_dsi_of_table[] = { > > - { .compatible = "allwinner,sun6i-a31-mipi-dsi" }, > > - { } > > + { > > + .compatible = "allwinner,sun6i-a31-mipi-dsi", > > + .data = &sun6i_a31_mipi_dsi, > > + }, > > + { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table); > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > index 3f4846f581ef..d791c9f6fccf 100644 > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > @@ -15,6 +15,10 @@ > > > > #define SUN6I_DSI_TCON_DIV 4 > > > > +struct sun6i_dsi_variant { > > + bool has_mod_clk; > > +}; > > + > > You could choose to put this above the probe function, since this isn't used > anywhere else, and a pointer field doesn't need the full definition. Okay. Will it be okay to check the compatibility directly on probe using of_device_is_compatible? Jagan.
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index c958ca9bae63..8c4c541224dd 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -11,6 +11,7 @@ #include <linux/crc-ccitt.h> #include <linux/module.h> #include <linux/of_address.h> +#include <linux/of_device.h> #include <linux/phy/phy-mipi-dphy.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> @@ -1093,6 +1094,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev) dsi->dev = dev; dsi->host.ops = &sun6i_dsi_host_ops; dsi->host.dev = dev; + dsi->variant = of_device_get_match_data(dev); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(dev, res); @@ -1120,17 +1122,20 @@ static int sun6i_dsi_probe(struct platform_device *pdev) return PTR_ERR(dsi->reset); } - 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); + 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); + } } /* * In order to operate properly, that clock seems to be always * set to 297MHz. */ - clk_set_rate_exclusive(dsi->mod_clk, 297000000); + if (dsi->variant->has_mod_clk) + clk_set_rate_exclusive(dsi->mod_clk, 297000000); dsi->dphy = devm_phy_get(dev, "dphy"); if (IS_ERR(dsi->dphy)) { @@ -1160,7 +1165,8 @@ static int sun6i_dsi_probe(struct platform_device *pdev) err_pm_disable: pm_runtime_disable(dev); err_unprotect_clk: - clk_rate_exclusive_put(dsi->mod_clk); + if (dsi->variant->has_mod_clk) + clk_rate_exclusive_put(dsi->mod_clk); return ret; } @@ -1172,7 +1178,8 @@ static int sun6i_dsi_remove(struct platform_device *pdev) component_del(&pdev->dev, &sun6i_dsi_ops); mipi_dsi_host_unregister(&dsi->host); pm_runtime_disable(dev); - clk_rate_exclusive_put(dsi->mod_clk); + if (dsi->variant->has_mod_clk) + clk_rate_exclusive_put(dsi->mod_clk); return 0; } @@ -1189,7 +1196,8 @@ static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev) } reset_control_deassert(dsi->reset); - clk_prepare_enable(dsi->mod_clk); + if (dsi->variant->has_mod_clk) + clk_prepare_enable(dsi->mod_clk); /* * Enable the DSI block. @@ -1217,7 +1225,8 @@ static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev) { struct sun6i_dsi *dsi = dev_get_drvdata(dev); - clk_disable_unprepare(dsi->mod_clk); + if (dsi->variant->has_mod_clk) + clk_disable_unprepare(dsi->mod_clk); reset_control_assert(dsi->reset); regulator_disable(dsi->regulator); @@ -1230,9 +1239,16 @@ static const struct dev_pm_ops sun6i_dsi_pm_ops = { NULL) }; +static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi = { + .has_mod_clk = true, +}; + static const struct of_device_id sun6i_dsi_of_table[] = { - { .compatible = "allwinner,sun6i-a31-mipi-dsi" }, - { } + { + .compatible = "allwinner,sun6i-a31-mipi-dsi", + .data = &sun6i_a31_mipi_dsi, + }, + { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table); diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index 3f4846f581ef..d791c9f6fccf 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -15,6 +15,10 @@ #define SUN6I_DSI_TCON_DIV 4 +struct sun6i_dsi_variant { + bool has_mod_clk; +}; + struct sun6i_dsi { struct drm_connector connector; struct drm_encoder encoder; @@ -31,6 +35,7 @@ struct sun6i_dsi { struct sun4i_drv *drv; struct mipi_dsi_device *device; struct drm_panel *panel; + const struct sun6i_dsi_variant *variant; }; static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)