Re: [PATCH drm-dp 4/4] drm/hisilicon/hibmc: add dp module in hibmc

[Yongbang Shi <shiyongbang@huawei.com>]

Okay, I'll fix them.
Thanks,
Baihan

> On Mon, 21 Oct 2024 at 14:54, s00452708 <shiyongbang@huawei.com> wrote:
>> Thanks, I will modify codes according to your comments, and I also
>> replied some questions or reasons why I did it below.
>>
>>> On Mon, Sep 30, 2024 at 06:06:10PM +0800, shiyongbang wrote:
>>>> From: baihan li <libaihan@huawei.com>
>>>>
>>>> To support DP interface displaying in hibmc driver. Add
>>>> a encoder and connector for DP modual.
>>>>
>>>> Signed-off-by: baihan li <libaihan@huawei.com>
>>>> ---
>>>>    drivers/gpu/drm/hisilicon/hibmc/Makefile      |   2 +-
>>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    | 195 ++++++++++++++++++
>>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  17 +-
>>>>    .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   5 +
>>>>    4 files changed, 217 insertions(+), 2 deletions(-)
>>>>    create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c
>>>>
> [...]
>
>>>> +
>>>> +static int hibmc_dp_connector_get_modes(struct drm_connector *connector)
>>>> +{
>>>> +    int count;
>>>> +
>>>> +    count = drm_add_modes_noedid(connector, connector->dev->mode_config.max_width,
>>>> +                                 connector->dev->mode_config.max_height);
>>>> +    drm_set_preferred_mode(connector, 800, 600); /* default 800x600 */
>>> What? Please parse EDID instead.
>>> I'll add aux over i2c r/w and get edid functions in next patch.
> At least please mention that it's a temporal stub which will be changed later.
>
>>>> +
>>>> +    return count;
>>>> +}
>>>> +
> [...]
>
>>>> @@ -116,10 +120,17 @@ static int hibmc_kms_init(struct hibmc_drm_private *priv)
>>>>               return ret;
>>>>       }
>>>>
>>>> +    /* if DP existed, init DP */
>>>> +    if ((readl(priv->mmio + DP_HOST_SERDES_CTRL) &
>>>> +         DP_HOST_SERDES_CTRL_MASK) == DP_HOST_SERDES_CTRL_VAL) {
>>>> +            ret = hibmc_dp_init(priv);
>>>> +            if (ret)
>>>> +                    drm_err(dev, "failed to init dp: %d\n", ret);
>>>> +    }
>>>> +
>>>>       ret = hibmc_vdac_init(priv);
>>>>       if (ret) {
>>>>               drm_err(dev, "failed to init vdac: %d\n", ret);
>>>> -            return ret;
>>> Why?
>>> We have two display cables, if VGA cannot work, this change makes DP
>>> still work.
> But that has nothing to do with init errors. If initialising (aka
> probing) VGA fails, then the driver init should fail. At the runtime
> the VGA and DP should be independent and it should be possible to
> drive just one output, that's true.
>