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