From: Matthias Brugger <matthias.bgg@gmail.com> To: Sean Wang <sean.wang@mediatek.com>, matthias.bgg@kernel.org Cc: ulrich.hecht+renesas@gmail.com, laurent.pinchart@ideasonboard.com, ck.hu@mediatek.com, p.zabel@pengutronix.de, airlied@linux.ie, robh+dt@kernel.org, mark.rutland@arm.com, mturquette@baylibre.com, sboyd@codeaurora.org, lee.jones@linaro.org, davem@davemloft.net, gregkh@linuxfoundation.org, mchehab@kernel.org, rdunlap@infradead.org, pi-cheng.chen@linaro.org, linux-clk@vger.kernel.org, linux@armlinux.org.uk, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, Matthias Brugger <mbrugger@suse.com> Subject: Re: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev Date: Wed, 18 Jul 2018 09:27:48 +0200 [thread overview] Message-ID: <50c93f6d-c546-1479-62c1-adc0b40d5dab@gmail.com> (raw) In-Reply-To: <1531882655.8953.83.camel@mtkswgap22> Hi Sean, On 18/07/18 04:57, Sean Wang wrote: > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: >> From: Matthias Brugger <mbrugger@suse.com> >> >> The MMSYS subsystem includes clocks and drm components. >> This patch adds an initailization path through a platform device >> for the clock part, so that both drivers get probed from the same >> device tree compatible. >> > > Sorry for that I should have a response earlier for the series. > > Some points I felt they're not exactly right and should be fixed up > before we're moving on > > Currently, drm driver have a wrong reference to the dt-binding, > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > for the subsystem exporting clock and reset line such common resource to > its sub-devices. Every subsystem has a similar shape. I hope mmsys > shouldn't be an exception. I'm not sure if I understand what you mean. You proposed to use a platform device [1] and now you say we should change the DTS? What do you mean by "drm driver has a wrong reference to the dt-binding"? How would you want to name it and most important, what would be your approach to have backwards compatibility to older device tree bindings? > > DRM device needs to have its own dt-binding show how connections between > DRM components being made and its node should be put under mmsys node. > > In this way, it becomes easy to see how the topology of the subsystem is > and grows, like a tree "device tree", instead of hiding the details in > the implementation. > > The similar example we already did for audsys on mt2701 and mt7623 as > below > > audsys: clock-controller@11220000 { > compatible = "mediatek,mt7623-audsys", > "mediatek,mt2701-audsys", > "syscon"; > ... > > afe: audio-controller { > compatible = "mediatek,mt7623-audio", > "mediatek,mt2701-audio"; > > ... > }; > }; > I think this approach is not working as, due to backwards compatibility, we have to make sure that the DRM driver gets probed with the mmsys binding. Regards, Matthias > > Sean > > >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> --- >> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ >> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> index dd249cf5121e..c946aea722e5 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { >> .ext_path = mt2701_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), >> .shadow_register = true, >> + .clk_drv_name = "clk-mt2701-mm", >> }; >> >> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), >> .ext_path = mt8173_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), >> + .clk_drv_name = "clk-mt8173-mm", >> }; >> >> static int mtk_drm_kms_init(struct drm_device *drm) >> @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) >> if (IS_ERR(private->config_regs)) >> return PTR_ERR(private->config_regs); >> >> + if (private->data->clk_drv_name) { >> + private->clk_dev = platform_device_register_data(dev, >> + private->data->clk_drv_name, -1, >> + NULL, 0); >> + >> + if (IS_ERR(private->clk_dev)) { >> + pr_err("failed to register %s platform device\n", >> + private->data->clk_drv_name); >> + >> + return PTR_ERR(private->clk_dev); >> + } >> + } >> + >> /* Iterate over sibling DISP function blocks */ >> for_each_child_of_node(dev->of_node->parent, node) { >> const struct of_device_id *of_id; >> @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) >> for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) >> of_node_put(private->comp_node[i]); >> >> + if (private->clk_dev) >> + platform_device_unregister(private->clk_dev); >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> index 86cec19193c4..200eee5de419 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { >> const enum mtk_ddp_comp_id *ext_path; >> unsigned int ext_len; >> bool shadow_register; >> + const char *clk_drv_name; >> }; >> >> struct mtk_drm_private { >> struct drm_device *drm; >> struct device *dma_dev; >> + struct platform_device *clk_dev; >> >> unsigned int num_pipes; >> > >
WARNING: multiple messages have this Message-ID (diff)
From: matthias.bgg@gmail.com (Matthias Brugger) To: linux-arm-kernel@lists.infradead.org Subject: [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev Date: Wed, 18 Jul 2018 09:27:48 +0200 [thread overview] Message-ID: <50c93f6d-c546-1479-62c1-adc0b40d5dab@gmail.com> (raw) In-Reply-To: <1531882655.8953.83.camel@mtkswgap22> Hi Sean, On 18/07/18 04:57, Sean Wang wrote: > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg at kernel.org wrote: >> From: Matthias Brugger <mbrugger@suse.com> >> >> The MMSYS subsystem includes clocks and drm components. >> This patch adds an initailization path through a platform device >> for the clock part, so that both drivers get probed from the same >> device tree compatible. >> > > Sorry for that I should have a response earlier for the series. > > Some points I felt they're not exactly right and should be fixed up > before we're moving on > > Currently, drm driver have a wrong reference to the dt-binding, > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > for the subsystem exporting clock and reset line such common resource to > its sub-devices. Every subsystem has a similar shape. I hope mmsys > shouldn't be an exception. I'm not sure if I understand what you mean. You proposed to use a platform device [1] and now you say we should change the DTS? What do you mean by "drm driver has a wrong reference to the dt-binding"? How would you want to name it and most important, what would be your approach to have backwards compatibility to older device tree bindings? > > DRM device needs to have its own dt-binding show how connections between > DRM components being made and its node should be put under mmsys node. > > In this way, it becomes easy to see how the topology of the subsystem is > and grows, like a tree "device tree", instead of hiding the details in > the implementation. > > The similar example we already did for audsys on mt2701 and mt7623 as > below > > audsys: clock-controller at 11220000 { > compatible = "mediatek,mt7623-audsys", > "mediatek,mt2701-audsys", > "syscon"; > ... > > afe: audio-controller { > compatible = "mediatek,mt7623-audio", > "mediatek,mt2701-audio"; > > ... > }; > }; > I think this approach is not working as, due to backwards compatibility, we have to make sure that the DRM driver gets probed with the mmsys binding. Regards, Matthias > > Sean > > >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> --- >> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ >> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> index dd249cf5121e..c946aea722e5 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { >> .ext_path = mt2701_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), >> .shadow_register = true, >> + .clk_drv_name = "clk-mt2701-mm", >> }; >> >> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), >> .ext_path = mt8173_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), >> + .clk_drv_name = "clk-mt8173-mm", >> }; >> >> static int mtk_drm_kms_init(struct drm_device *drm) >> @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) >> if (IS_ERR(private->config_regs)) >> return PTR_ERR(private->config_regs); >> >> + if (private->data->clk_drv_name) { >> + private->clk_dev = platform_device_register_data(dev, >> + private->data->clk_drv_name, -1, >> + NULL, 0); >> + >> + if (IS_ERR(private->clk_dev)) { >> + pr_err("failed to register %s platform device\n", >> + private->data->clk_drv_name); >> + >> + return PTR_ERR(private->clk_dev); >> + } >> + } >> + >> /* Iterate over sibling DISP function blocks */ >> for_each_child_of_node(dev->of_node->parent, node) { >> const struct of_device_id *of_id; >> @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) >> for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) >> of_node_put(private->comp_node[i]); >> >> + if (private->clk_dev) >> + platform_device_unregister(private->clk_dev); >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> index 86cec19193c4..200eee5de419 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { >> const enum mtk_ddp_comp_id *ext_path; >> unsigned int ext_len; >> bool shadow_register; >> + const char *clk_drv_name; >> }; >> >> struct mtk_drm_private { >> struct drm_device *drm; >> struct device *dma_dev; >> + struct platform_device *clk_dev; >> >> unsigned int num_pipes; >> > >
next prev parent reply other threads:[~2018-07-18 7:27 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-17 22:03 [resend PATCH v4 0/5] arm/arm64: mediatek: Fix mmsys device probing matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-17 22:03 ` [resend PATCH v4 1/5] drm/mediatek: Use regmap for register access matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-17 22:03 ` [resend PATCH v4 2/5] clk: mediatek: mt2701-mmsys: switch to platform device probing matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-17 22:03 ` [resend PATCH v4 3/5] clk: mediatek: mt8173: switch mmsys " matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-17 22:03 ` [resend PATCH v4 4/5] drm/mediatek: Add support for mmsys through a pdev matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-18 2:57 ` Sean Wang 2018-07-18 2:57 ` Sean Wang 2018-07-18 2:57 ` Sean Wang 2018-07-18 7:27 ` Matthias Brugger [this message] 2018-07-18 7:27 ` Matthias Brugger 2018-07-18 7:28 ` Matthias Brugger 2018-07-18 7:28 ` Matthias Brugger 2018-07-18 9:16 ` Sean Wang 2018-07-18 9:16 ` Sean Wang 2018-07-18 9:16 ` Sean Wang 2018-07-18 8:05 ` Laurent Pinchart 2018-07-18 8:05 ` Laurent Pinchart 2018-07-18 8:05 ` Laurent Pinchart 2018-07-18 10:06 ` Sean Wang 2018-07-18 10:06 ` Sean Wang 2018-07-18 10:06 ` Sean Wang 2018-07-26 7:35 ` Stephen Boyd 2018-07-26 7:35 ` Stephen Boyd 2018-07-26 7:35 ` Stephen Boyd 2018-07-26 7:35 ` Stephen Boyd 2018-10-12 18:23 ` Stephen Boyd 2018-10-12 18:23 ` Stephen Boyd 2018-07-17 22:03 ` [resend PATCH v4 5/5] drm: mediatek: Omit warning on probe defers matthias.bgg 2018-07-17 22:03 ` matthias.bgg at kernel.org 2018-07-17 22:03 ` matthias.bgg 2018-07-18 3:04 ` Sean Wang 2018-07-18 3:04 ` Sean Wang 2018-07-18 3:04 ` Sean Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=50c93f6d-c546-1479-62c1-adc0b40d5dab@gmail.com \ --to=matthias.bgg@gmail.com \ --cc=airlied@linux.ie \ --cc=ck.hu@mediatek.com \ --cc=davem@davemloft.net \ --cc=dri-devel@lists.freedesktop.org \ --cc=gregkh@linuxfoundation.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=matthias.bgg@kernel.org \ --cc=mbrugger@suse.com \ --cc=mchehab@kernel.org \ --cc=mturquette@baylibre.com \ --cc=p.zabel@pengutronix.de \ --cc=pi-cheng.chen@linaro.org \ --cc=rdunlap@infradead.org \ --cc=robh+dt@kernel.org \ --cc=sboyd@codeaurora.org \ --cc=sean.wang@mediatek.com \ --cc=ulrich.hecht+renesas@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.