Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort driver
From: AngeloGioacchino Del Regno
Date: Wed Apr 13 2022 - 03:51:04 EST
Il 12/04/22 11:46, Guillaume Ranquet ha scritto:
On Mon, 28 Mar 2022 11:14, AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
Il 28/03/22 00:39, Guillaume Ranquet ha scritto:
From: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>.
Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
Signed-off-by: Guillaume Ranquet <granquet@xxxxxxxxxxxx>
Reported-by: kernel test robot <lkp@xxxxxxxxx>
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