Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort driver

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 12/04/22 11:46, Guillaume Ranquet ha scritto:
> On Mon, 28 Mar 2022 11:14, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>> Il 28/03/22 00:39, Guillaume Ranquet ha scritto:
>>> From: Markus Schneider-Pargmann <msp@baylibre.com>
>>>
>>> This patch adds a DisplayPort driver for the Mediatek mt8195 SoC.
>>>
>>> It supports the mt8195, the embedded DisplayPort units. It offers
>>> DisplayPort 1.4 with up to 4 lanes.
>>>
>>> The driver shares its iomap range with the mtk-dp-phy driver using
>>> the regmap/syscon facility.
>>>
>>> This driver is based on an initial version by
>>> Jason-JH.Lin <jason-jh.lin@mediatek.com>.
>>>
>>> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
>>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> Hello Guillaume,
>> as you know, there's some more work to do on this driver.
>>
>> I will also mention here, not on the code, that at this point, your
>> mtk_dp_aux_transfer() function has something VERY similar to function
>> drm_dp_dpcd_access(), so I really believe that you can instead use
>> functions drm_dp_dpcd_read() and drm_dp_dpcd_write(), avoiding code
>> duplication around.
>>
>> Please check drivers/gpu/drm/dp/drm_dp.c.
>>
> 
> This is already in my TODO list as this has been suggested by Rex earlier.
> 
>>> ---
>>>    drivers/gpu/drm/mediatek/Kconfig       |    8 +
>>>    drivers/gpu/drm/mediatek/Makefile      |    2 +
>>>    drivers/gpu/drm/mediatek/mtk_dp.c      | 2221 ++++++++++++++++++++++++
>>>    drivers/gpu/drm/mediatek/mtk_dp_reg.h  |  568 ++++++
>>>    drivers/gpu/drm/mediatek/mtk_drm_drv.c |    1 +
>>>    drivers/gpu/drm/mediatek/mtk_drm_drv.h |    1 +
>>>    6 files changed, 2801 insertions(+)
>>>    create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c
>>>    create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
>>> index 2976d21e9a34..03ffa9b896c3 100644
>>> --- a/drivers/gpu/drm/mediatek/Kconfig
>>> +++ b/drivers/gpu/drm/mediatek/Kconfig
>>> @@ -28,3 +28,11 @@ config DRM_MEDIATEK_HDMI
>>>    	select PHY_MTK_HDMI
>>>    	help
>>>    	  DRM/KMS HDMI driver for Mediatek SoCs
>>> +
>>> +config MTK_DPTX_SUPPORT
>>
>> Actually, I think that the best would be DRM_MEDIATEK_DP_TX or DRM_MEDIATEK_DP...
>> ...also, ordering is important, please!
>>
> I will update the name.
> What do you mean by ordering? do you expect the configs to be ordered
> alphabetically?
> 

Yes, correct. Configuration options shall be alphabetically ordered.
Obviously, the same applies for Makefile :)

>>> +	tristate "DRM DPTX Support for Mediatek SoCs"
>>> +	depends on DRM_MEDIATEK
>>> +	select PHY_MTK_DP
>>> +	select DRM_DP_HELPER
>>> +	help
>>> +	  DRM/KMS Display Port driver for Mediatek SoCs.
>>> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
>>> index 29098d7c8307..d86a6406055e 100644
>>> --- a/drivers/gpu/drm/mediatek/Makefile
>>> +++ b/drivers/gpu/drm/mediatek/Makefile
>>> @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
>>>    			  mtk_hdmi_ddc.o
>>>
>>>    obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
>>> +
>>> +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
>>> new file mode 100644
>>> index 000000000000..7cd8459cf719
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c

..snip..

>>> +
>>> +#define MTK_UPD_BITS_OR_OUT(mtk_dp, offset, val, mask, ret, label) \
>>> +	do {\
>>> +		ret = mtk_dp_update_bits(mtk_dp, offset, val, mask); \
>>> +		if (ret) \
>>> +			goto label; \
>>> +	} while (0)
>>
>> I'm sorry, no offense - but this macro is a bit ugly...
>>
>> I think I understand why you have introduced it, but in my opinion this decreases
>> human readability a lot, I was even about to point out multiple functions that
>> you had unused labels before checking this macro, as that was totally unexpected...
>>
>> In my opinion, this should be open-coded everywhere... yes it makes the file a
>> bit fatter in terms of amount of text, but eh... it's life :)))
>>
>>
> No offense taken... I find the macro ugly too... but I couldn't think
> of anything less
> ugly... I'll bite the bullet and write all of the code then.
> 

I can't think of anything shorter, either.. so yep, sometimes writing 'em all
is simply the best option.

>>> +
>>
>> ....snip....
>>
>>> +
>>> +static int mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
>>> +				   enum mtk_dp_color_format color_format)
>>> +{


..snip..

>>> +
>>> +static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp,
>>> +			   struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	int ret = 0;
>>> +	void __iomem *base;
>>> +
>>> +	base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(base))
>>> +		return PTR_ERR(base);
>>> +
>>> +	mtk_dp->regs = syscon_node_to_regmap(dev->of_node);
>>> +	if (IS_ERR(mtk_dp->regs))
>>> +		return PTR_ERR(mtk_dp->regs);
>>> +
>>> +	//TODO: optional clock?
>>
>> Well, if it's optional, you should use devm_clk_get_optional(), meaning
>> that......
>>
>>> +	mtk_dp->dp_tx_clk = devm_clk_get(dev, "faxi");
>>> +	if (IS_ERR(mtk_dp->dp_tx_clk)) {
>>> +		ret = PTR_ERR(mtk_dp->dp_tx_clk);
>>> +		dev_info(dev, "Failed to get dptx clock: %d\n", ret);
>>> +		mtk_dp->dp_tx_clk = NULL;
>>
>> ...I shouldn't see this snippet of code, as it should be a bit different... :)
>>
> It is still unclear to me if this should be optional or not, hence the 'TODO'
> 

Thinking about this a bit more - this driver is for MT8195 only, for now... and
the DP cannot work without the AXI clock being up... logically.

I'd say, at this point, to make this mandatory.

If any newer platform will make this optional, we can either assign clk26m (ugh!)
or we can get back to this one and convert this to devm_clk_get_optional().

Don't forget that we can also manage this in devicetree bindings, where we can
for example declare a property to be optional for one or more compatibles, so
there's no problem if we'd be doing such a change in the future, I think.

>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>
>> ..snip..
>>
>>> +static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>> +					 struct drm_bridge_state *old_state)
>>> +{
>>> +	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
>>> +
>>> +	mtk_dp_video_mute(mtk_dp, true);
>>> +	mtk_dp->state = MTK_DP_STATE_IDLE;
>>> +	mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
>>> +
>>> +	mtk_dp->enabled = false;
>>
>>> +	msleep(100);
>>
>> 100 milliseconds is an eternity, why are we sleeping for *so long* here?
>>
>> Please, either add a comment that fully explains the resons for that, or
>> remove the sleep entirely: to my eyes, it doesn't look like this sleep is
>> really needed for anything important because here you are disabling the
>> bridge and *powering off* the IP entirely.
>>
> This sleep can be removed.
> I will also make an effort to document all the other delays in the code for v10.
> 

Thanks for that.

(snip)

>>> +
>>
>>
> 
> Thx a lot for your review,
> Guillaume.


You're welcome!

Cheers,
Angelo

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek