Re: [PATCH 1/3] drm/rockchip: dw_hdmi_qp: Add platform ctrl callback

From: "Heiko Stübner" <heiko@sntech.de>
To: Andy Yan <andyshrk@163.com>
Cc: cristian.ciocaltea@collabora.com, krzk+dt@kernel.org,
	mripard@kernel.org, hjc@rock-chips.com,
	devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
	Andy Yan <andy.yan@rock-chips.com>
Subject: Re: [PATCH 1/3] drm/rockchip: dw_hdmi_qp: Add platform ctrl callback
Date: Thu, 02 Jan 2025 12:51:58 +0100	[thread overview]
Message-ID: <2175680.OBFZWjSADL@diego> (raw)
In-Reply-To: <20241225103741.364597-2-andyshrk@163.com>

Hi Andy,

Am Mittwoch, 25. Dezember 2024, 11:37:29 CET schrieb Andy Yan:
> From: Andy Yan <andy.yan@rock-chips.com>
> 
> There are some control bits for IO and interrupts status scattered
> across different GRF on differt SOC.
> Add platform callback for this IO setting and interrupts status
> handling.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> 
> ---
> 
>  .../gpu/drm/rockchip/dw_hdmi_qp-rockchip.c    | 81 ++++++++++++-------
>  1 file changed, 54 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index c36fc130b734..b21e868e7c16 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -62,6 +62,12 @@ struct rockchip_hdmi_qp {
>  	int port_id;
>  };
>  
> +struct rockchip_hdmi_qp_ctrl_ops {
> +	void (*io_init)(struct rockchip_hdmi_qp *hdmi);
> +	irqreturn_t (*irq_callback)(int irq, void *dev_id);
> +	irqreturn_t (*hardirq_callback)(int irq, void *dev_id);
> +};
> +
>  static struct rockchip_hdmi_qp *to_rockchip_hdmi_qp(struct drm_encoder *encoder)
>  {
>  	struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> @@ -226,9 +232,47 @@ static irqreturn_t dw_hdmi_qp_rk3588_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void dw_hdmi_qp_rk3588_io_init(struct rockchip_hdmi_qp *hdmi)
> +{
> +	u32 val;
> +
> +	val = HIWORD_UPDATE(RK3588_SCLIN_MASK, RK3588_SCLIN_MASK) |
> +	      HIWORD_UPDATE(RK3588_SDAIN_MASK, RK3588_SDAIN_MASK) |
> +	      HIWORD_UPDATE(RK3588_MODE_MASK, RK3588_MODE_MASK) |
> +	      HIWORD_UPDATE(RK3588_I2S_SEL_MASK, RK3588_I2S_SEL_MASK);
> +

nit: below val = ... and regmap_write don't have a blank line between them
which makes sense to show that they belong together. So the blank above
can probably also go away.

> +	regmap_write(hdmi->vo_regmap,
> +		     hdmi->port_id ? RK3588_GRF_VO1_CON6 : RK3588_GRF_VO1_CON3,
> +		     val);
> +
> +	val = HIWORD_UPDATE(RK3588_SET_HPD_PATH_MASK, RK3588_SET_HPD_PATH_MASK);
> +	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON7, val);
> +
> +	if (hdmi->port_id)
> +		val = HIWORD_UPDATE(RK3588_HDMI1_GRANT_SEL,
> +				RK3588_HDMI1_GRANT_SEL);
> +	else
> +		val = HIWORD_UPDATE(RK3588_HDMI0_GRANT_SEL,
> +				RK3588_HDMI0_GRANT_SEL);
> +	regmap_write(hdmi->vo_regmap, RK3588_GRF_VO1_CON9, val);
> +
> +	if (hdmi->port_id)
> +		val = HIWORD_UPDATE(RK3588_HDMI1_HPD_INT_MSK, RK3588_HDMI1_HPD_INT_MSK);
> +	else
> +		val = HIWORD_UPDATE(RK3588_HDMI0_HPD_INT_MSK, RK3588_HDMI0_HPD_INT_MSK);
> +	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
> +}
> +
> +static const struct rockchip_hdmi_qp_ctrl_ops rk3588_hdmi_ctrl_ops = {
> +	.io_init		= dw_hdmi_qp_rk3588_io_init,
> +	.irq_callback	        = dw_hdmi_qp_rk3588_irq,
> +	.hardirq_callback	= dw_hdmi_qp_rk3588_hardirq,
> +};
> +
>  struct rockchip_hdmi_qp_cfg {
>  	unsigned int num_ports;
>  	unsigned int port_ids[MAX_HDMI_PORT_NUM];
> +	const struct rockchip_hdmi_qp_ctrl_ops *ctrl_ops;
>  	const struct dw_hdmi_qp_phy_ops *phy_ops;
>  };
>  
> @@ -238,6 +282,7 @@ static const struct rockchip_hdmi_qp_cfg rk3588_hdmi_cfg = {
>  		0xfde80000,
>  		0xfdea0000,
>  	},
> +	.ctrl_ops = &rk3588_hdmi_ctrl_ops,
>  	.phy_ops = &rk3588_hdmi_phy_ops,
>  };
>  
> @@ -282,6 +327,12 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	if (!cfg)
>  		return -ENODEV;
>  
> +	if (!cfg->ctrl_ops || !cfg->ctrl_ops->io_init ||
> +	    !cfg->ctrl_ops->irq_callback || !cfg->ctrl_ops->irq_callback) {

cfg->ctrl_ops->irq_callback is checked twice, I guess one of them
should've been ctrl_ops->hardirq_callback?

Patch looks good otherwise.

Thanks
Heiko