Re: [PATCH v2 4/7] drm/rockchip: dw_hdmi: add inno hdmi phy ops

[Heiko Stuebner]

Hi Algea,

Am Samstag, 30. September 2017, 09:44:46 CET schrieb Algea Cao:
> Because some RK chips use inno hdmi phy, such as RK3328, we add
> inno hdmi phy ops.
> 
> Signed-off-by: Algea Cao <algea.cao@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 110 +++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 09c77f9..7658b2f 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -12,7 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> -
> +#include <linux/phy/phy.h>
>  #include <drm/drm_of.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -26,6 +26,18 @@
>  #define RK3288_HDMI_LCDC_SEL		BIT(4)
>  #define RK3399_GRF_SOC_CON20		0x6250
>  #define RK3399_HDMI_LCDC_SEL		BIT(6)
> +#define RK3328_GRF_SOC_CON2		0x0408
> +#define RK3328_DDC_MASK_EN		((3 << 10) | (3 << (10 + 16)))
> +#define RK3328_GRF_SOC_CON3		0x040c
> +#define RK3328_IO_CTRL_BY_HDMI		(0xf0000000 | BIT(13) | BIT(12))
> +#define RK3328_GRF_SOC_CON4		0x0410
> +#define RK3328_IO_3V_DOMAIN		(7 << (9 + 16))
> +#define RK3328_IO_5V_DOMAIN		((7 << 9) | (3 << (9 + 16)))

This is slightly confusing. (9+16) is obviously the write-enable-mask, so
shouldn't the write-enable-mask not at least cover the changed bits?


> +static enum drm_connector_status
> +inno_dw_hdmi_phy_read_hpd(struct dw_hdmi *dw_hdmi, void *data)
> +{
> +	struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data;
> +	enum drm_connector_status status;
> +
> +	status = dw_hdmi_phy_read_hpd(dw_hdmi, data);
> +
> +	if (hdmi->dev_type == RK3228_HDMI)
> +		return status;

There is no need to encode the functionality for both variants
(and possibly later ones) into one function.

As Neil said in his reply to patch2, we don't really want to carry
that dev_type property, so why don't you just define this as
rk3328_inno_phy_read_hpd and then use it like 

static const struct dw_hdmi_phy_ops rk3228_dw_hdmi_phy_ops = {
	.init		= inno_dw_hdmi_phy_init,
	.disable	= inno_dw_hdmi_phy_disable,
	.read_hpd	= dw_hdmi_phy_read_hpd,
};

static const struct dw_hdmi_phy_ops rk3328_dw_hdmi_phy_ops = {
	.init		= inno_dw_hdmi_phy_init,
	.disable	= inno_dw_hdmi_phy_disable,
	.read_hpd	= rk3328_inno_phy_read_hpd,
};

> +	if (status == connector_status_connected)
> +		regmap_write(hdmi->regmap,
> +			     RK3328_GRF_SOC_CON4,
> +			     RK3328_IO_5V_DOMAIN);
> +	else
> +		regmap_write(hdmi->regmap,
> +			     RK3328_GRF_SOC_CON4,
> +			     RK3328_IO_3V_DOMAIN);

That should definitely get a comment explaining what this is doing and
why :-) . Especially as it seems related to the plug/unplug events.

According to the TRM, the hdmiphy IP block has its own interrupt. Can
you tell me if it is firing on hotplug events? Because I'm wondering if
these GRF settings wouldn't be better live in the hdmiphy driver
[if it gets an irq on plug/unplug], which in turn would make the phy
handling in dw_hdmi a lot easier.

> +	return status;
> +}
> +

[...]

> @@ -361,7 +453,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  		return -ENOMEM;
>  
>  	match = of_match_node(dw_hdmi_rockchip_dt_ids, pdev->dev.of_node);
> -	plat_data = match->data;
> +	plat_data = (struct dw_hdmi_plat_data *)match->data;
>  	hdmi->dev = &pdev->dev;
>  	hdmi->chip_data = plat_data->phy_data;
>  	hdmi->dev_type = plat_data->dev_type;
> @@ -383,6 +475,18 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  		return ret;
>  	}
>  
> +	if (hdmi->dev_type == RK3328_HDMI || hdmi->dev_type == RK3228_HDMI) {
> +		hdmi->phy = devm_phy_get(dev, "hdmi_phy");
> +		if (IS_ERR(hdmi->phy)) {
> +			ret = PTR_ERR(hdmi->phy);
> +			dev_err(dev, "failed to get phy: %d\n", ret);
> +			return ret;
> +		}
> +		plat_data->phy_data = hdmi;
> +		ret = inno_dw_hdmi_init(hdmi);
> +		if (ret < 0)
> +			return ret;
> +	}

You could also just declare it optional in the binding, try to get the phy
via devm_phy_optional_get and if it's NULL just assume that it's the
regular internal phy as with previous socs. That way you can drop the
dev-type-check.

Aka, if a phy is defined we are pretty sure we want to use that one :-) .


Heiko