This series prepares the sun8i HDMI PHY driver for supporting the new custom PHY in the Allwinner D1 SoC. No functional change intended here.
This series was tested on D1 and H3.
Samuel Holland (6): drm/sun4i: sun8i-hdmi-phy: Use of_device_get_match_data drm/sun4i: sun8i-hdmi-phy: Use devm_platform_ioremap_resource drm/sun4i: sun8i-hdmi-phy: Used device-managed clocks/resets drm/sun4i: sun8i-hdmi-phy: Support multiple custom PHY ops drm/sun4i: sun8i-hdmi-phy: Separate A83T and H3 PHY ops drm/sun4i: sun8i-hdmi-phy: Group PHY ops functions by generation
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 9 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 252 ++++++++++--------------- 2 files changed, 101 insertions(+), 160 deletions(-)
Now that the HDMI PHY is using a platform driver, we can use the usual helper function for getting the variant structure.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 2 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index bffe1b9cd3dc..0adbfac6eb31 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -173,7 +173,7 @@ struct sun8i_hdmi_phy { unsigned int rcal; struct regmap *regs; struct reset_control *rst_phy; - struct sun8i_hdmi_phy_variant *variant; + const struct sun8i_hdmi_phy_variant *variant; };
struct sun8i_dw_hdmi_quirks { diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 5e2b0175df36..7b901aef789a 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -565,7 +565,7 @@ void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy) void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, struct dw_hdmi_plat_data *plat_data) { - struct sun8i_hdmi_phy_variant *variant = phy->variant; + const struct sun8i_hdmi_phy_variant *variant = phy->variant;
if (variant->is_custom_phy) { plat_data->phy_ops = &sun8i_hdmi_phy_ops; @@ -672,7 +672,6 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node)
static int sun8i_hdmi_phy_probe(struct platform_device *pdev) { - const struct of_device_id *match; struct device *dev = &pdev->dev; struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; @@ -680,17 +679,11 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) void __iomem *regs; int ret;
- match = of_match_node(sun8i_hdmi_phy_of_table, node); - if (!match) { - dev_err(dev, "Incompatible HDMI PHY\n"); - return -EINVAL; - } - phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); if (!phy) return -ENOMEM;
- phy->variant = (struct sun8i_hdmi_phy_variant *)match->data; + phy->variant = of_device_get_match_data(dev); phy->dev = dev;
ret = of_address_to_resource(node, 0, &res);
The struct resource is not used for anything else, so we can simplify the code a bit by using the helper function.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 7b901aef789a..1effa30bfe62 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -675,7 +675,6 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; - struct resource res; void __iomem *regs; int ret;
@@ -686,13 +685,7 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) phy->variant = of_device_get_match_data(dev); phy->dev = dev;
- ret = of_address_to_resource(node, 0, &res); - if (ret) { - dev_err(dev, "phy: Couldn't get our resources\n"); - return ret; - } - - regs = devm_ioremap_resource(dev, &res); + regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(regs)) { dev_err(dev, "Couldn't map the HDMI PHY registers\n"); return PTR_ERR(regs);
Now that the HDMI PHY is using a platform driver, it can use device- managed resources. Use these, as well as the dev_err_probe helper, to simplify the probe function and get rid of the remove function.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 100 ++++++++----------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 1effa30bfe62..1351e633d485 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -673,10 +673,8 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node) static int sun8i_hdmi_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; void __iomem *regs; - int ret;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); if (!phy) @@ -686,88 +684,50 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) phy->dev = dev;
regs = devm_platform_ioremap_resource(pdev, 0); - if (IS_ERR(regs)) { - dev_err(dev, "Couldn't map the HDMI PHY registers\n"); - return PTR_ERR(regs); - } + if (IS_ERR(regs)) + return dev_err_probe(dev, PTR_ERR(regs), + "Couldn't map the HDMI PHY registers\n");
phy->regs = devm_regmap_init_mmio(dev, regs, &sun8i_hdmi_phy_regmap_config); - if (IS_ERR(phy->regs)) { - dev_err(dev, "Couldn't create the HDMI PHY regmap\n"); - return PTR_ERR(phy->regs); - } + if (IS_ERR(phy->regs)) + return dev_err_probe(dev, PTR_ERR(phy->regs), + "Couldn't create the HDMI PHY regmap\n");
- phy->clk_bus = of_clk_get_by_name(node, "bus"); - if (IS_ERR(phy->clk_bus)) { - dev_err(dev, "Could not get bus clock\n"); - return PTR_ERR(phy->clk_bus); - } - - phy->clk_mod = of_clk_get_by_name(node, "mod"); - if (IS_ERR(phy->clk_mod)) { - dev_err(dev, "Could not get mod clock\n"); - ret = PTR_ERR(phy->clk_mod); - goto err_put_clk_bus; - } + phy->clk_bus = devm_clk_get(dev, "bus"); + if (IS_ERR(phy->clk_bus)) + return dev_err_probe(dev, PTR_ERR(phy->clk_bus), + "Could not get bus clock\n");
- if (phy->variant->has_phy_clk) { - phy->clk_pll0 = of_clk_get_by_name(node, "pll-0"); - if (IS_ERR(phy->clk_pll0)) { - dev_err(dev, "Could not get pll-0 clock\n"); - ret = PTR_ERR(phy->clk_pll0); - goto err_put_clk_mod; - } - - if (phy->variant->has_second_pll) { - phy->clk_pll1 = of_clk_get_by_name(node, "pll-1"); - if (IS_ERR(phy->clk_pll1)) { - dev_err(dev, "Could not get pll-1 clock\n"); - ret = PTR_ERR(phy->clk_pll1); - goto err_put_clk_pll0; - } - } - } + phy->clk_mod = devm_clk_get(dev, "mod"); + if (IS_ERR(phy->clk_mod)) + return dev_err_probe(dev, PTR_ERR(phy->clk_mod), + "Could not get mod clock\n");
- phy->rst_phy = of_reset_control_get_shared(node, "phy"); - if (IS_ERR(phy->rst_phy)) { - dev_err(dev, "Could not get phy reset control\n"); - ret = PTR_ERR(phy->rst_phy); - goto err_put_clk_pll1; - } + if (phy->variant->has_phy_clk) + phy->clk_pll0 = devm_clk_get(dev, "pll-0"); + if (IS_ERR(phy->clk_pll0)) + return dev_err_probe(dev, PTR_ERR(phy->clk_pll0), + "Could not get pll-0 clock\n"); + + if (phy->variant->has_second_pll) + phy->clk_pll1 = devm_clk_get(dev, "pll-1"); + if (IS_ERR(phy->clk_pll1)) + return dev_err_probe(dev, PTR_ERR(phy->clk_pll1), + "Could not get pll-1 clock\n"); + + phy->rst_phy = devm_reset_control_get_shared(dev, "phy"); + if (IS_ERR(phy->rst_phy)) + return dev_err_probe(dev, PTR_ERR(phy->rst_phy), + "Could not get phy reset control\n");
platform_set_drvdata(pdev, phy);
return 0; - -err_put_clk_pll1: - clk_put(phy->clk_pll1); -err_put_clk_pll0: - clk_put(phy->clk_pll0); -err_put_clk_mod: - clk_put(phy->clk_mod); -err_put_clk_bus: - clk_put(phy->clk_bus); - - return ret; -} - -static int sun8i_hdmi_phy_remove(struct platform_device *pdev) -{ - struct sun8i_hdmi_phy *phy = platform_get_drvdata(pdev); - - reset_control_put(phy->rst_phy); - - clk_put(phy->clk_pll0); - clk_put(phy->clk_pll1); - clk_put(phy->clk_mod); - clk_put(phy->clk_bus); - return 0; }
struct platform_driver sun8i_hdmi_phy_driver = { .probe = sun8i_hdmi_phy_probe, - .remove = sun8i_hdmi_phy_remove, .driver = { .name = "sun8i-hdmi-phy", .of_match_table = sun8i_hdmi_phy_of_table,
Hi,
On Mon, Apr 11, 2022 at 11:35:08PM -0500, Samuel Holland wrote:
Now that the HDMI PHY is using a platform driver, it can use device- managed resources. Use these, as well as the dev_err_probe helper, to simplify the probe function and get rid of the remove function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 100 ++++++++----------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 1effa30bfe62..1351e633d485 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -673,10 +673,8 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node) static int sun8i_hdmi_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;
struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; void __iomem *regs;
int ret;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); if (!phy)
@@ -686,88 +684,50 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) phy->dev = dev;
regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(regs)) {
dev_err(dev, "Couldn't map the HDMI PHY registers\n");return PTR_ERR(regs);- }
if (IS_ERR(regs))
return dev_err_probe(dev, PTR_ERR(regs),"Couldn't map the HDMI PHY registers\n");phy->regs = devm_regmap_init_mmio(dev, regs, &sun8i_hdmi_phy_regmap_config);
- if (IS_ERR(phy->regs)) {
dev_err(dev, "Couldn't create the HDMI PHY regmap\n");return PTR_ERR(phy->regs);- }
- if (IS_ERR(phy->regs))
return dev_err_probe(dev, PTR_ERR(phy->regs),"Couldn't create the HDMI PHY regmap\n");
- phy->clk_bus = of_clk_get_by_name(node, "bus");
- if (IS_ERR(phy->clk_bus)) {
dev_err(dev, "Could not get bus clock\n");return PTR_ERR(phy->clk_bus);- }
- phy->clk_mod = of_clk_get_by_name(node, "mod");
- if (IS_ERR(phy->clk_mod)) {
dev_err(dev, "Could not get mod clock\n");ret = PTR_ERR(phy->clk_mod);goto err_put_clk_bus;- }
- phy->clk_bus = devm_clk_get(dev, "bus");
- if (IS_ERR(phy->clk_bus))
return dev_err_probe(dev, PTR_ERR(phy->clk_bus),"Could not get bus clock\n");
- if (phy->variant->has_phy_clk) {
phy->clk_pll0 = of_clk_get_by_name(node, "pll-0");if (IS_ERR(phy->clk_pll0)) {dev_err(dev, "Could not get pll-0 clock\n");ret = PTR_ERR(phy->clk_pll0);goto err_put_clk_mod;}if (phy->variant->has_second_pll) {phy->clk_pll1 = of_clk_get_by_name(node, "pll-1");if (IS_ERR(phy->clk_pll1)) {dev_err(dev, "Could not get pll-1 clock\n");ret = PTR_ERR(phy->clk_pll1);goto err_put_clk_pll0;}}- }
- phy->clk_mod = devm_clk_get(dev, "mod");
- if (IS_ERR(phy->clk_mod))
return dev_err_probe(dev, PTR_ERR(phy->clk_mod),"Could not get mod clock\n");
- phy->rst_phy = of_reset_control_get_shared(node, "phy");
- if (IS_ERR(phy->rst_phy)) {
dev_err(dev, "Could not get phy reset control\n");ret = PTR_ERR(phy->rst_phy);goto err_put_clk_pll1;- }
- if (phy->variant->has_phy_clk)
phy->clk_pll0 = devm_clk_get(dev, "pll-0");- if (IS_ERR(phy->clk_pll0))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll0),"Could not get pll-0 clock\n");- if (phy->variant->has_second_pll)
phy->clk_pll1 = devm_clk_get(dev, "pll-1");- if (IS_ERR(phy->clk_pll1))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll1),"Could not get pll-1 clock\n");- phy->rst_phy = devm_reset_control_get_shared(dev, "phy");
- if (IS_ERR(phy->rst_phy))
return dev_err_probe(dev, PTR_ERR(phy->rst_phy),"Could not get phy reset control\n");
I find the old construct clearer with the imbricated blocks.
Otherwise, the rest of the series looks fine, thanks! Maxime
On 4/12/22 8:23 AM, Maxime Ripard wrote:
Hi,
On Mon, Apr 11, 2022 at 11:35:08PM -0500, Samuel Holland wrote:
Now that the HDMI PHY is using a platform driver, it can use device- managed resources. Use these, as well as the dev_err_probe helper, to simplify the probe function and get rid of the remove function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 100 ++++++++----------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 1effa30bfe62..1351e633d485 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -673,10 +673,8 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node) static int sun8i_hdmi_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;
struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; void __iomem *regs;
int ret;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); if (!phy)
@@ -686,88 +684,50 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) phy->dev = dev;
regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(regs)) {
dev_err(dev, "Couldn't map the HDMI PHY registers\n");return PTR_ERR(regs);- }
if (IS_ERR(regs))
return dev_err_probe(dev, PTR_ERR(regs),"Couldn't map the HDMI PHY registers\n");phy->regs = devm_regmap_init_mmio(dev, regs, &sun8i_hdmi_phy_regmap_config);
- if (IS_ERR(phy->regs)) {
dev_err(dev, "Couldn't create the HDMI PHY regmap\n");return PTR_ERR(phy->regs);- }
- if (IS_ERR(phy->regs))
return dev_err_probe(dev, PTR_ERR(phy->regs),"Couldn't create the HDMI PHY regmap\n");
- phy->clk_bus = of_clk_get_by_name(node, "bus");
- if (IS_ERR(phy->clk_bus)) {
dev_err(dev, "Could not get bus clock\n");return PTR_ERR(phy->clk_bus);- }
- phy->clk_mod = of_clk_get_by_name(node, "mod");
- if (IS_ERR(phy->clk_mod)) {
dev_err(dev, "Could not get mod clock\n");ret = PTR_ERR(phy->clk_mod);goto err_put_clk_bus;- }
- phy->clk_bus = devm_clk_get(dev, "bus");
- if (IS_ERR(phy->clk_bus))
return dev_err_probe(dev, PTR_ERR(phy->clk_bus),"Could not get bus clock\n");
- if (phy->variant->has_phy_clk) {
phy->clk_pll0 = of_clk_get_by_name(node, "pll-0");if (IS_ERR(phy->clk_pll0)) {dev_err(dev, "Could not get pll-0 clock\n");ret = PTR_ERR(phy->clk_pll0);goto err_put_clk_mod;}if (phy->variant->has_second_pll) {phy->clk_pll1 = of_clk_get_by_name(node, "pll-1");if (IS_ERR(phy->clk_pll1)) {dev_err(dev, "Could not get pll-1 clock\n");ret = PTR_ERR(phy->clk_pll1);goto err_put_clk_pll0;}}- }
- phy->clk_mod = devm_clk_get(dev, "mod");
- if (IS_ERR(phy->clk_mod))
return dev_err_probe(dev, PTR_ERR(phy->clk_mod),"Could not get mod clock\n");
- phy->rst_phy = of_reset_control_get_shared(node, "phy");
- if (IS_ERR(phy->rst_phy)) {
dev_err(dev, "Could not get phy reset control\n");ret = PTR_ERR(phy->rst_phy);goto err_put_clk_pll1;- }
- if (phy->variant->has_phy_clk)
phy->clk_pll0 = devm_clk_get(dev, "pll-0");- if (IS_ERR(phy->clk_pll0))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll0),"Could not get pll-0 clock\n");- if (phy->variant->has_second_pll)
phy->clk_pll1 = devm_clk_get(dev, "pll-1");- if (IS_ERR(phy->clk_pll1))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll1),"Could not get pll-1 clock\n");- phy->rst_phy = devm_reset_control_get_shared(dev, "phy");
- if (IS_ERR(phy->rst_phy))
return dev_err_probe(dev, PTR_ERR(phy->rst_phy),"Could not get phy reset control\n");I find the old construct clearer with the imbricated blocks.
I'm not sure what you mean here. Are you suggesting braces around the dev_err_probe statements? Or do you want me to put the error handling for variant-specific resources inside the variant checks? Please clarify.
Regards, Samuel
Hi Samuel,
Sorry for the (very) late answer
On Tue, Apr 12, 2022 at 06:34:40PM -0500, Samuel Holland wrote:
On 4/12/22 8:23 AM, Maxime Ripard wrote:
Hi,
On Mon, Apr 11, 2022 at 11:35:08PM -0500, Samuel Holland wrote:
Now that the HDMI PHY is using a platform driver, it can use device- managed resources. Use these, as well as the dev_err_probe helper, to simplify the probe function and get rid of the remove function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 100 ++++++++----------------- 1 file changed, 30 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 1effa30bfe62..1351e633d485 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -673,10 +673,8 @@ int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node) static int sun8i_hdmi_phy_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;
struct device_node *node = dev->of_node; struct sun8i_hdmi_phy *phy; void __iomem *regs;
int ret;
phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); if (!phy)
@@ -686,88 +684,50 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) phy->dev = dev;
regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(regs)) {
dev_err(dev, "Couldn't map the HDMI PHY registers\n");return PTR_ERR(regs);- }
if (IS_ERR(regs))
return dev_err_probe(dev, PTR_ERR(regs),"Couldn't map the HDMI PHY registers\n");phy->regs = devm_regmap_init_mmio(dev, regs, &sun8i_hdmi_phy_regmap_config);
- if (IS_ERR(phy->regs)) {
dev_err(dev, "Couldn't create the HDMI PHY regmap\n");return PTR_ERR(phy->regs);- }
- if (IS_ERR(phy->regs))
return dev_err_probe(dev, PTR_ERR(phy->regs),"Couldn't create the HDMI PHY regmap\n");
- phy->clk_bus = of_clk_get_by_name(node, "bus");
- if (IS_ERR(phy->clk_bus)) {
dev_err(dev, "Could not get bus clock\n");return PTR_ERR(phy->clk_bus);- }
- phy->clk_mod = of_clk_get_by_name(node, "mod");
- if (IS_ERR(phy->clk_mod)) {
dev_err(dev, "Could not get mod clock\n");ret = PTR_ERR(phy->clk_mod);goto err_put_clk_bus;- }
- phy->clk_bus = devm_clk_get(dev, "bus");
- if (IS_ERR(phy->clk_bus))
return dev_err_probe(dev, PTR_ERR(phy->clk_bus),"Could not get bus clock\n");
- if (phy->variant->has_phy_clk) {
phy->clk_pll0 = of_clk_get_by_name(node, "pll-0");if (IS_ERR(phy->clk_pll0)) {dev_err(dev, "Could not get pll-0 clock\n");ret = PTR_ERR(phy->clk_pll0);goto err_put_clk_mod;}if (phy->variant->has_second_pll) {phy->clk_pll1 = of_clk_get_by_name(node, "pll-1");if (IS_ERR(phy->clk_pll1)) {dev_err(dev, "Could not get pll-1 clock\n");ret = PTR_ERR(phy->clk_pll1);goto err_put_clk_pll0;}}- }
- phy->clk_mod = devm_clk_get(dev, "mod");
- if (IS_ERR(phy->clk_mod))
return dev_err_probe(dev, PTR_ERR(phy->clk_mod),"Could not get mod clock\n");
- phy->rst_phy = of_reset_control_get_shared(node, "phy");
- if (IS_ERR(phy->rst_phy)) {
dev_err(dev, "Could not get phy reset control\n");ret = PTR_ERR(phy->rst_phy);goto err_put_clk_pll1;- }
- if (phy->variant->has_phy_clk)
phy->clk_pll0 = devm_clk_get(dev, "pll-0");- if (IS_ERR(phy->clk_pll0))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll0),"Could not get pll-0 clock\n");- if (phy->variant->has_second_pll)
phy->clk_pll1 = devm_clk_get(dev, "pll-1");- if (IS_ERR(phy->clk_pll1))
return dev_err_probe(dev, PTR_ERR(phy->clk_pll1),"Could not get pll-1 clock\n");- phy->rst_phy = devm_reset_control_get_shared(dev, "phy");
- if (IS_ERR(phy->rst_phy))
return dev_err_probe(dev, PTR_ERR(phy->rst_phy),"Could not get phy reset control\n");I find the old construct clearer with the imbricated blocks.
I'm not sure what you mean here. Are you suggesting braces around the dev_err_probe statements? Or do you want me to put the error handling for variant-specific resources inside the variant checks? Please clarify.
I meant that we went, for example, from:
if (phy->variant->has_phy_clk) { phy->clk_pll0 = devm_clk_get(dev, "pll-0"); if (IS_ERR(phy->clk_pll0)) { ... } }
to
if (phy->variant->has_phy_clk) phy->clk_pll0 = devm_clk_get(dev, "pll-0"); if (IS_ERR(phy->clk_pll0)) { ... }
Which relies on the fact that phy->clk_pll0 is initialized to !IS_ERR(...), which we never explicitly enforced. I find the first and original code clearer for that aspect (since we only use that value if it was set), and less fragile.
Maxime
The D1 SoC comes with a new custom HDMI PHY, which does not share any registers with the existing custom PHY. So it needs a new set of ops. Instead of providing a flag in the variant structure, provide the ops themselves.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 2 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 0adbfac6eb31..f0b1aaad27d9 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -151,10 +151,10 @@ struct sun8i_hdmi_phy; struct sun8i_hdmi_phy_variant { bool has_phy_clk; bool has_second_pll; - unsigned int is_custom_phy : 1; const struct dw_hdmi_curr_ctrl *cur_ctr; const struct dw_hdmi_mpll_config *mpll_cfg; const struct dw_hdmi_phy_config *phy_cfg; + const struct dw_hdmi_phy_ops *phy_ops; void (*phy_init)(struct sun8i_hdmi_phy *phy); void (*phy_disable)(struct dw_hdmi *hdmi, struct sun8i_hdmi_phy *phy); diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 1351e633d485..71062e4e8666 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -567,8 +567,8 @@ void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, { const struct sun8i_hdmi_phy_variant *variant = phy->variant;
- if (variant->is_custom_phy) { - plat_data->phy_ops = &sun8i_hdmi_phy_ops; + if (variant->phy_ops) { + plat_data->phy_ops = variant->phy_ops; plat_data->phy_name = "sun8i_dw_hdmi_phy"; plat_data->phy_data = phy; } else { @@ -587,7 +587,7 @@ static const struct regmap_config sun8i_hdmi_phy_regmap_config = { };
static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { - .is_custom_phy = true, + .phy_ops = &sun8i_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_a83t, .phy_disable = &sun8i_hdmi_phy_disable_a83t, .phy_config = &sun8i_hdmi_phy_config_a83t, @@ -595,7 +595,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = {
static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { .has_phy_clk = true, - .is_custom_phy = true, + .phy_ops = &sun8i_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, .phy_disable = &sun8i_hdmi_phy_disable_h3, .phy_config = &sun8i_hdmi_phy_config_h3, @@ -604,7 +604,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { .has_phy_clk = true, .has_second_pll = true, - .is_custom_phy = true, + .phy_ops = &sun8i_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, .phy_disable = &sun8i_hdmi_phy_disable_h3, .phy_config = &sun8i_hdmi_phy_config_h3, @@ -612,7 +612,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = {
static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { .has_phy_clk = true, - .is_custom_phy = true, + .phy_ops = &sun8i_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, .phy_disable = &sun8i_hdmi_phy_disable_h3, .phy_config = &sun8i_hdmi_phy_config_h3,
Since the driver already needs to support multiple sets of ops, we can drop the mid-layer used by the A83T and H3 PHYs. They share only a small amount of code; factor this out as sun8i_hdmi_phy_set_polarity.
For clarity, this commit keeps the existing function order.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 5 -- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 89 +++++++++++++------------- 2 files changed, 46 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index f0b1aaad27d9..f082b8ecfe2c 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -156,11 +156,6 @@ struct sun8i_hdmi_phy_variant { const struct dw_hdmi_phy_config *phy_cfg; const struct dw_hdmi_phy_ops *phy_ops; void (*phy_init)(struct sun8i_hdmi_phy *phy); - void (*phy_disable)(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy); - int (*phy_config)(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy, - unsigned int clk_rate); };
struct sun8i_hdmi_phy { diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 71062e4e8666..5be5c360ca7d 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -123,10 +123,18 @@ static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { { ~0UL, 0x0000, 0x0000, 0x0000} };
-static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy, - unsigned int clk_rate) +static void sun8i_hdmi_phy_set_polarity(struct sun8i_hdmi_phy *phy, + const struct drm_display_mode *mode); + +static int sun8i_a83t_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, + const struct drm_display_info *display, + const struct drm_display_mode *mode) { + unsigned int clk_rate = mode->crtc_clock * 1000; + struct sun8i_hdmi_phy *phy = data; + + sun8i_hdmi_phy_set_polarity(phy, mode); + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); @@ -185,10 +193,12 @@ static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, return 0; }
-static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy, - unsigned int clk_rate) +static int sun8i_h3_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, + const struct drm_display_info *display, + const struct drm_display_mode *mode) { + unsigned int clk_rate = mode->crtc_clock * 1000; + struct sun8i_hdmi_phy *phy = data; u32 pll_cfg1_init; u32 pll_cfg2_init; u32 ana_cfg1_end; @@ -197,6 +207,11 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, u32 b_offset = 0; u32 val;
+ if (phy->variant->has_phy_clk) + clk_set_rate(phy->clk_phy, clk_rate); + + sun8i_hdmi_phy_set_polarity(phy, mode); + /* bandwidth / frequency independent settings */
pll_cfg1_init = SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN | @@ -333,11 +348,9 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi, return 0; }
-static int sun8i_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, - const struct drm_display_info *display, - const struct drm_display_mode *mode) +static void sun8i_hdmi_phy_set_polarity(struct sun8i_hdmi_phy *phy, + const struct drm_display_mode *mode) { - struct sun8i_hdmi_phy *phy = (struct sun8i_hdmi_phy *)data; u32 val = 0;
if (mode->flags & DRM_MODE_FLAG_NHSYNC) @@ -348,16 +361,12 @@ static int sun8i_hdmi_phy_config(struct dw_hdmi *hdmi, void *data,
regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_DBG_CTRL_REG, SUN8I_HDMI_PHY_DBG_CTRL_POL_MASK, val); - - if (phy->variant->has_phy_clk) - clk_set_rate(phy->clk_phy, mode->crtc_clock * 1000); - - return phy->variant->phy_config(hdmi, phy, mode->crtc_clock * 1000); };
-static void sun8i_hdmi_phy_disable_a83t(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy) +static void sun8i_a83t_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) { + struct sun8i_hdmi_phy *phy = data; + dw_hdmi_phy_gen2_txpwron(hdmi, 0); dw_hdmi_phy_gen2_pddq(hdmi, 1);
@@ -365,9 +374,10 @@ static void sun8i_hdmi_phy_disable_a83t(struct dw_hdmi *hdmi, SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, 0); }
-static void sun8i_hdmi_phy_disable_h3(struct dw_hdmi *hdmi, - struct sun8i_hdmi_phy *phy) +static void sun8i_h3_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) { + struct sun8i_hdmi_phy *phy = data; + regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, SUN8I_HDMI_PHY_ANA_CFG1_LDOEN | SUN8I_HDMI_PHY_ANA_CFG1_ENVBS | @@ -375,19 +385,20 @@ static void sun8i_hdmi_phy_disable_h3(struct dw_hdmi *hdmi, regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); }
-static void sun8i_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) -{ - struct sun8i_hdmi_phy *phy = (struct sun8i_hdmi_phy *)data; - - phy->variant->phy_disable(hdmi, phy); -} +static const struct dw_hdmi_phy_ops sun8i_a83t_hdmi_phy_ops = { + .init = sun8i_a83t_hdmi_phy_config, + .disable = sun8i_a83t_hdmi_phy_disable, + .read_hpd = dw_hdmi_phy_read_hpd, + .update_hpd = dw_hdmi_phy_update_hpd, + .setup_hpd = dw_hdmi_phy_setup_hpd, +};
-static const struct dw_hdmi_phy_ops sun8i_hdmi_phy_ops = { - .init = &sun8i_hdmi_phy_config, - .disable = &sun8i_hdmi_phy_disable, - .read_hpd = &dw_hdmi_phy_read_hpd, - .update_hpd = &dw_hdmi_phy_update_hpd, - .setup_hpd = &dw_hdmi_phy_setup_hpd, +static const struct dw_hdmi_phy_ops sun8i_h3_hdmi_phy_ops = { + .init = sun8i_h3_hdmi_phy_config, + .disable = sun8i_h3_hdmi_phy_disable, + .read_hpd = dw_hdmi_phy_read_hpd, + .update_hpd = dw_hdmi_phy_update_hpd, + .setup_hpd = dw_hdmi_phy_setup_hpd, };
static void sun8i_hdmi_phy_unlock(struct sun8i_hdmi_phy *phy) @@ -587,35 +598,27 @@ static const struct regmap_config sun8i_hdmi_phy_regmap_config = { };
static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { - .phy_ops = &sun8i_hdmi_phy_ops, + .phy_ops = &sun8i_a83t_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_a83t, - .phy_disable = &sun8i_hdmi_phy_disable_a83t, - .phy_config = &sun8i_hdmi_phy_config_a83t, };
static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { .has_phy_clk = true, - .phy_ops = &sun8i_hdmi_phy_ops, + .phy_ops = &sun8i_h3_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, - .phy_disable = &sun8i_hdmi_phy_disable_h3, - .phy_config = &sun8i_hdmi_phy_config_h3, };
static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { .has_phy_clk = true, .has_second_pll = true, - .phy_ops = &sun8i_hdmi_phy_ops, + .phy_ops = &sun8i_h3_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, - .phy_disable = &sun8i_hdmi_phy_disable_h3, - .phy_config = &sun8i_hdmi_phy_config_h3, };
static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { .has_phy_clk = true, - .phy_ops = &sun8i_hdmi_phy_ops, + .phy_ops = &sun8i_h3_hdmi_phy_ops, .phy_init = &sun8i_hdmi_phy_init_h3, - .phy_disable = &sun8i_hdmi_phy_disable_h3, - .phy_config = &sun8i_hdmi_phy_config_h3, };
static const struct sun8i_hdmi_phy_variant sun50i_h6_hdmi_phy = {
Now that the PHY ops are separated, sort them topologically, with the common sun8i_hdmi_phy_set_polarity helper at the top. No function contents are changed in this commit.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 67 ++++++++++++-------------- 1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 5be5c360ca7d..cc239106ba49 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -124,7 +124,19 @@ static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { };
static void sun8i_hdmi_phy_set_polarity(struct sun8i_hdmi_phy *phy, - const struct drm_display_mode *mode); + const struct drm_display_mode *mode) +{ + u32 val = 0; + + if (mode->flags & DRM_MODE_FLAG_NHSYNC) + val |= SUN8I_HDMI_PHY_DBG_CTRL_POL_NHSYNC; + + if (mode->flags & DRM_MODE_FLAG_NVSYNC) + val |= SUN8I_HDMI_PHY_DBG_CTRL_POL_NVSYNC; + + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_DBG_CTRL_REG, + SUN8I_HDMI_PHY_DBG_CTRL_POL_MASK, val); +};
static int sun8i_a83t_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, const struct drm_display_info *display, @@ -193,6 +205,25 @@ static int sun8i_a83t_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, return 0; }
+static void sun8i_a83t_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) +{ + struct sun8i_hdmi_phy *phy = data; + + dw_hdmi_phy_gen2_txpwron(hdmi, 0); + dw_hdmi_phy_gen2_pddq(hdmi, 1); + + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, 0); +} + +static const struct dw_hdmi_phy_ops sun8i_a83t_hdmi_phy_ops = { + .init = sun8i_a83t_hdmi_phy_config, + .disable = sun8i_a83t_hdmi_phy_disable, + .read_hpd = dw_hdmi_phy_read_hpd, + .update_hpd = dw_hdmi_phy_update_hpd, + .setup_hpd = dw_hdmi_phy_setup_hpd, +}; + static int sun8i_h3_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, const struct drm_display_info *display, const struct drm_display_mode *mode) @@ -348,32 +379,6 @@ static int sun8i_h3_hdmi_phy_config(struct dw_hdmi *hdmi, void *data, return 0; }
-static void sun8i_hdmi_phy_set_polarity(struct sun8i_hdmi_phy *phy, - const struct drm_display_mode *mode) -{ - u32 val = 0; - - if (mode->flags & DRM_MODE_FLAG_NHSYNC) - val |= SUN8I_HDMI_PHY_DBG_CTRL_POL_NHSYNC; - - if (mode->flags & DRM_MODE_FLAG_NVSYNC) - val |= SUN8I_HDMI_PHY_DBG_CTRL_POL_NVSYNC; - - regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_DBG_CTRL_REG, - SUN8I_HDMI_PHY_DBG_CTRL_POL_MASK, val); -}; - -static void sun8i_a83t_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) -{ - struct sun8i_hdmi_phy *phy = data; - - dw_hdmi_phy_gen2_txpwron(hdmi, 0); - dw_hdmi_phy_gen2_pddq(hdmi, 1); - - regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, - SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, 0); -} - static void sun8i_h3_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) { struct sun8i_hdmi_phy *phy = data; @@ -385,14 +390,6 @@ static void sun8i_h3_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); }
-static const struct dw_hdmi_phy_ops sun8i_a83t_hdmi_phy_ops = { - .init = sun8i_a83t_hdmi_phy_config, - .disable = sun8i_a83t_hdmi_phy_disable, - .read_hpd = dw_hdmi_phy_read_hpd, - .update_hpd = dw_hdmi_phy_update_hpd, - .setup_hpd = dw_hdmi_phy_setup_hpd, -}; - static const struct dw_hdmi_phy_ops sun8i_h3_hdmi_phy_ops = { .init = sun8i_h3_hdmi_phy_config, .disable = sun8i_h3_hdmi_phy_disable,
dri-devel@lists.freedesktop.org
-
Maxime Ripard -
Samuel Holland