linux-kernel - Re:Re: [PATCH 1/3] drm/rockchip: dw_hdmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2f23fa41.82d9.1943ac12d62.Coremail.andyshrk@163.com>
Date: Mon, 6 Jan 2025 16:35:26 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: Heiko Stübner <heiko@...ech.de>
Cc: cristian.ciocaltea@...labora.com, krzk+dt@...nel.org, mripard@...nel.org, 
	hjc@...k-chips.com, devicetree@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-rockchip@...ts.infradead.org, 
	"Andy Yan" <andy.yan@...k-chips.com>
Subject: Re:Re: [PATCH 1/3] drm/rockchip: dw_hdmi_qp: Add platform ctrl
 callback


Hi Heiko,

At 2025-01-02 19:51:58, "Heiko Stübner" <heiko@...ech.de> wrote:
>Hi Andy,
>
>Am Mittwoch, 25. Dezember 2024, 11:37:29 CET schrieb Andy Yan:
>> From: Andy Yan <andy.yan@...k-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@...k-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
>
>
>