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

["Andy Yan" <andyshrk@163.com>]

Hi Heiko,

At 2025-01-02 19:51:58, "Heiko Stübner" <heiko@sntech.de> wrote:
>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.

Okay, it will be removed in V2.
>
>> +	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?

Yes，Thank you for catching it。
>
>Patch looks good otherwise.
>
>Thanks
>Heiko
>
>
>