| Message ID | 20191207224740.24536-13-matthias.bgg@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | arm/arm64: mediatek: Fix mmsys device probing | expand |
Hi, Matthias: On Sat, 2019-12-07 at 23:47 +0100, 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. You've switched mt6779 and mt6797 clock driver to platform device probing, but you does not probe then in drm driver, so your design is to let mmsys clock not work if drm driver is not enabled? It's fine for me because it seems that drm driver is the only user of mmsys clock now. Regards, CK > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 24 ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 210455e9f46c..5ada74d8d0c9 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -186,6 +186,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 mt2712_mmsys_driver_data = { > @@ -195,6 +196,7 @@ static const struct mtk_mmsys_driver_data mt2712_mmsys_driver_data = { > .ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext), > .third_path = mt2712_mtk_ddp_third, > .third_len = ARRAY_SIZE(mt2712_mtk_ddp_third), > + .clk_drv_name = "clk-mt2712-mm", > }; > > static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > @@ -202,6 +204,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) > @@ -499,6 +502,24 @@ static int mtk_drm_probe(struct platform_device *pdev) > INIT_WORK(&private->commit.work, mtk_atomic_work); > private->data = of_device_get_match_data(dev); > > + /* > + * MMSYS includes apart from components management a block providing > + * clocks for the subsystem. We probe this clock driver via a platform > + * device. > + */ > + 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)) { > + dev_err(dev, "failed to register %s platform device\n", > + private->data->clk_drv_name); > + > + return PTR_ERR(private->clk_dev); > + } > + } > + > private->config_regs = syscon_node_to_regmap(dev->of_node); > if (IS_ERR(private->config_regs)) > return PTR_ERR(private->config_regs); > @@ -605,6 +626,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 63a121577dcb..8fe9136adc38 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > @@ -29,11 +29,13 @@ struct mtk_mmsys_driver_data { > unsigned int third_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; >
Hi Matthias, Just one nit below and LGTM On 7/12/19 23:47, 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. > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 24 ++++++++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 210455e9f46c..5ada74d8d0c9 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -186,6 +186,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 mt2712_mmsys_driver_data = { > @@ -195,6 +196,7 @@ static const struct mtk_mmsys_driver_data mt2712_mmsys_driver_data = { > .ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext), > .third_path = mt2712_mtk_ddp_third, > .third_len = ARRAY_SIZE(mt2712_mtk_ddp_third), > + .clk_drv_name = "clk-mt2712-mm", > }; > > static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > @@ -202,6 +204,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) > @@ -499,6 +502,24 @@ static int mtk_drm_probe(struct platform_device *pdev) > INIT_WORK(&private->commit.work, mtk_atomic_work); > private->data = of_device_get_match_data(dev); > > + /* > + * MMSYS includes apart from components management a block providing > + * clocks for the subsystem. We probe this clock driver via a platform > + * device. > + */ > + if (private->data->clk_drv_name) { > + private->clk_dev = platform_device_register_data(dev, > + private->data->clk_drv_name, -1, > + NULL, 0); Replace -1 for PLATFORM_DEVID_NONE > + > + if (IS_ERR(private->clk_dev)) { > + dev_err(dev, "failed to register %s platform device\n", > + private->data->clk_drv_name); > + > + return PTR_ERR(private->clk_dev); > + } > + } > + > private->config_regs = syscon_node_to_regmap(dev->of_node); > if (IS_ERR(private->config_regs)) > return PTR_ERR(private->config_regs); > @@ -605,6 +626,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 63a121577dcb..8fe9136adc38 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > @@ -29,11 +29,13 @@ struct mtk_mmsys_driver_data { > unsigned int third_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; > >
On 09/12/2019 06:34, CK Hu wrote: > Hi, Matthias: > > On Sat, 2019-12-07 at 23:47 +0100, 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. > > You've switched mt6779 and mt6797 clock driver to platform device > probing, but you does not probe then in drm driver, so your design is to > let mmsys clock not work if drm driver is not enabled? It's fine for me > because it seems that drm driver is the only user of mmsys clock now. > Exactly. The mmsys clocks are only needed for the drm driver. Therefor we only probe them when probing the drm driver. Regards, Matthias > Regards, > CK > >> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> --- >> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 24 ++++++++++++++++++++++++ >> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> index 210455e9f46c..5ada74d8d0c9 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> @@ -186,6 +186,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 mt2712_mmsys_driver_data = { >> @@ -195,6 +196,7 @@ static const struct mtk_mmsys_driver_data mt2712_mmsys_driver_data = { >> .ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext), >> .third_path = mt2712_mtk_ddp_third, >> .third_len = ARRAY_SIZE(mt2712_mtk_ddp_third), >> + .clk_drv_name = "clk-mt2712-mm", >> }; >> >> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> @@ -202,6 +204,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) >> @@ -499,6 +502,24 @@ static int mtk_drm_probe(struct platform_device *pdev) >> INIT_WORK(&private->commit.work, mtk_atomic_work); >> private->data = of_device_get_match_data(dev); >> >> + /* >> + * MMSYS includes apart from components management a block providing >> + * clocks for the subsystem. We probe this clock driver via a platform >> + * device. >> + */ >> + 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)) { >> + dev_err(dev, "failed to register %s platform device\n", >> + private->data->clk_drv_name); >> + >> + return PTR_ERR(private->clk_dev); >> + } >> + } >> + >> private->config_regs = syscon_node_to_regmap(dev->of_node); >> if (IS_ERR(private->config_regs)) >> return PTR_ERR(private->config_regs); >> @@ -605,6 +626,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 63a121577dcb..8fe9136adc38 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> @@ -29,11 +29,13 @@ struct mtk_mmsys_driver_data { >> unsigned int third_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; >> > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek >
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 210455e9f46c..5ada74d8d0c9 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -186,6 +186,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 mt2712_mmsys_driver_data = { @@ -195,6 +196,7 @@ static const struct mtk_mmsys_driver_data mt2712_mmsys_driver_data = { .ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext), .third_path = mt2712_mtk_ddp_third, .third_len = ARRAY_SIZE(mt2712_mtk_ddp_third), + .clk_drv_name = "clk-mt2712-mm", }; static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { @@ -202,6 +204,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) @@ -499,6 +502,24 @@ static int mtk_drm_probe(struct platform_device *pdev) INIT_WORK(&private->commit.work, mtk_atomic_work); private->data = of_device_get_match_data(dev); + /* + * MMSYS includes apart from components management a block providing + * clocks for the subsystem. We probe this clock driver via a platform + * device. + */ + 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)) { + dev_err(dev, "failed to register %s platform device\n", + private->data->clk_drv_name); + + return PTR_ERR(private->clk_dev); + } + } + private->config_regs = syscon_node_to_regmap(dev->of_node); if (IS_ERR(private->config_regs)) return PTR_ERR(private->config_regs); @@ -605,6 +626,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 63a121577dcb..8fe9136adc38 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -29,11 +29,13 @@ struct mtk_mmsys_driver_data { unsigned int third_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;