Hi Inki,
Many Exynos DRM drivers uses global variables to represent associated devices in Exynos DRM internal framework. It is quite confusing, it adds data duplication and finally it does not allow to handle more than one device in system. It seems better to embed such structures in private context of the device.
The patchset is based on exynos_drm_next plus my patch [1]: 'drm/exynos: remove explicit encoder/connector de-initialization'.
If the patchset is OK for you I can prepare similar patches for other Exynos DRM components.
[1]: https://lkml.org/lkml/2014/9/22/148
Regards Andrzej
Andrzej Hajda (4): drm/exynos/dsi: remove global variable exynos_dsi_display drm/exynos/dsi: simplify device pointer evaluation drm/exynos/dsi: remove redundant encoder field drm/exynos/dsi: stop using display->ctx pointer
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 96 ++++++++++++++++----------------- 1 file changed, 47 insertions(+), 49 deletions(-)
exynos_dsi_display is used by internal Exynos DRM framework for representing pair encoder->connecter. As it should be mapped 1:1 to dsi private context it seems more reasonable to embed it directly in that context. As a result further code simplification will be possible. Moreover it will be possible to handle multiple DSI devices in the system.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 38 ++++++++++++++++----------------- 1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index acf7e9e..84e3808 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -268,6 +268,7 @@ struct exynos_dsi_driver_data { };
struct exynos_dsi { + struct exynos_drm_display display; struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_encoder *encoder; @@ -1531,10 +1532,6 @@ static struct exynos_drm_display_ops exynos_dsi_display_ops = { .dpms = exynos_dsi_dpms };
-static struct exynos_drm_display exynos_dsi_display = { - .type = EXYNOS_DISPLAY_TYPE_LCD, - .ops = &exynos_dsi_display_ops, -}; MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);
/* of_* functions will be removed after merge of of_graph patches */ @@ -1640,28 +1637,28 @@ end: static int exynos_dsi_bind(struct device *dev, struct device *master, void *data) { + struct exynos_drm_display *display = dev_get_drvdata(dev); + struct exynos_dsi *dsi = display->ctx; struct drm_device *drm_dev = data; - struct exynos_dsi *dsi; int ret;
- ret = exynos_drm_create_enc_conn(drm_dev, &exynos_dsi_display); + ret = exynos_drm_create_enc_conn(drm_dev, display); if (ret) { DRM_ERROR("Encoder create [%d] failed with %d\n", - exynos_dsi_display.type, ret); + display->type, ret); return ret; }
- dsi = exynos_dsi_display.ctx; - return mipi_dsi_host_register(&dsi->dsi_host); }
static void exynos_dsi_unbind(struct device *dev, struct device *master, void *data) { - struct exynos_dsi *dsi = exynos_dsi_display.ctx; + struct exynos_drm_display *display = dev_get_drvdata(dev); + struct exynos_dsi *dsi = display->ctx;
- exynos_dsi_dpms(&exynos_dsi_display, DRM_MODE_DPMS_OFF); + exynos_dsi_dpms(display, DRM_MODE_DPMS_OFF);
mipi_dsi_host_unregister(&dsi->dsi_host); } @@ -1673,22 +1670,23 @@ static const struct component_ops exynos_dsi_component_ops = {
static int exynos_dsi_probe(struct platform_device *pdev) { + struct device *dev = &pdev->dev; struct resource *res; struct exynos_dsi *dsi; int ret;
- ret = exynos_drm_component_add(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR, - exynos_dsi_display.type); + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); + if (!dsi) + return -ENOMEM; + + dsi->display.type = EXYNOS_DISPLAY_TYPE_LCD; + dsi->display.ops = &exynos_dsi_display_ops; + + ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CONNECTOR, + dsi->display.type); if (ret) return ret;
- dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL); - if (!dsi) { - dev_err(&pdev->dev, "failed to allocate dsi object.\n"); - ret = -ENOMEM; - goto err_del_component; - } - /* To be checked as invalid one */ dsi->te_gpio = -ENOENT;
The patch replaces multiple evaluation of device address with local variable.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 40 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 84e3808..39ec450 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1695,9 +1695,9 @@ static int exynos_dsi_probe(struct platform_device *pdev) INIT_LIST_HEAD(&dsi->transfer_list);
dsi->dsi_host.ops = &exynos_dsi_ops; - dsi->dsi_host.dev = &pdev->dev; + dsi->dsi_host.dev = dev;
- dsi->dev = &pdev->dev; + dsi->dev = dev; dsi->driver_data = exynos_dsi_get_driver_data(pdev);
ret = exynos_dsi_parse_dt(dsi); @@ -1706,70 +1706,70 @@ static int exynos_dsi_probe(struct platform_device *pdev)
dsi->supplies[0].supply = "vddcore"; dsi->supplies[1].supply = "vddio"; - ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(dsi->supplies), + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dsi->supplies), dsi->supplies); if (ret) { - dev_info(&pdev->dev, "failed to get regulators: %d\n", ret); + dev_info(dev, "failed to get regulators: %d\n", ret); return -EPROBE_DEFER; }
- dsi->pll_clk = devm_clk_get(&pdev->dev, "pll_clk"); + dsi->pll_clk = devm_clk_get(dev, "pll_clk"); if (IS_ERR(dsi->pll_clk)) { - dev_info(&pdev->dev, "failed to get dsi pll input clock\n"); + dev_info(dev, "failed to get dsi pll input clock\n"); ret = PTR_ERR(dsi->pll_clk); goto err_del_component; }
- dsi->bus_clk = devm_clk_get(&pdev->dev, "bus_clk"); + dsi->bus_clk = devm_clk_get(dev, "bus_clk"); if (IS_ERR(dsi->bus_clk)) { - dev_info(&pdev->dev, "failed to get dsi bus clock\n"); + dev_info(dev, "failed to get dsi bus clock\n"); ret = PTR_ERR(dsi->bus_clk); goto err_del_component; }
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - dsi->reg_base = devm_ioremap_resource(&pdev->dev, res); + dsi->reg_base = devm_ioremap_resource(dev, res); if (IS_ERR(dsi->reg_base)) { - dev_err(&pdev->dev, "failed to remap io region\n"); + dev_err(dev, "failed to remap io region\n"); ret = PTR_ERR(dsi->reg_base); goto err_del_component; }
- dsi->phy = devm_phy_get(&pdev->dev, "dsim"); + dsi->phy = devm_phy_get(dev, "dsim"); if (IS_ERR(dsi->phy)) { - dev_info(&pdev->dev, "failed to get dsim phy\n"); + dev_info(dev, "failed to get dsim phy\n"); ret = PTR_ERR(dsi->phy); goto err_del_component; }
dsi->irq = platform_get_irq(pdev, 0); if (dsi->irq < 0) { - dev_err(&pdev->dev, "failed to request dsi irq resource\n"); + dev_err(dev, "failed to request dsi irq resource\n"); ret = dsi->irq; goto err_del_component; }
irq_set_status_flags(dsi->irq, IRQ_NOAUTOEN); - ret = devm_request_threaded_irq(&pdev->dev, dsi->irq, NULL, + ret = devm_request_threaded_irq(dev, dsi->irq, NULL, exynos_dsi_irq, IRQF_ONESHOT, - dev_name(&pdev->dev), dsi); + dev_name(dev), dsi); if (ret) { - dev_err(&pdev->dev, "failed to request dsi irq\n"); + dev_err(dev, "failed to request dsi irq\n"); goto err_del_component; }
- exynos_dsi_display.ctx = dsi; + dsi->display.ctx = dsi;
- platform_set_drvdata(pdev, &exynos_dsi_display); + platform_set_drvdata(pdev, &dsi->display);
- ret = component_add(&pdev->dev, &exynos_dsi_component_ops); + ret = component_add(dev, &exynos_dsi_component_ops); if (ret) goto err_del_component;
return ret;
err_del_component: - exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR); + exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CONNECTOR); return ret; }
The patch removes redundant encoder field from private DSI context.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 39ec450..634904a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -271,7 +271,6 @@ struct exynos_dsi { struct exynos_drm_display display; struct mipi_dsi_host dsi_host; struct drm_connector connector; - struct drm_encoder *encoder; struct device_node *panel_node; struct drm_panel *panel; struct device *dev; @@ -1105,7 +1104,7 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id) static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id) { struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id; - struct drm_encoder *encoder = dsi->encoder; + struct drm_encoder *encoder = dsi->display.encoder;
if (dsi->state & DSIM_STATE_ENABLED) exynos_drm_crtc_te_handler(encoder->crtc); @@ -1475,7 +1474,7 @@ exynos_dsi_best_encoder(struct drm_connector *connector) { struct exynos_dsi *dsi = connector_to_dsi(connector);
- return dsi->encoder; + return dsi->display.encoder; }
static struct drm_connector_helper_funcs exynos_dsi_connector_helper_funcs = { @@ -1491,8 +1490,6 @@ static int exynos_dsi_create_connector(struct exynos_drm_display *display, struct drm_connector *connector = &dsi->connector; int ret;
- dsi->encoder = encoder; - connector->polled = DRM_CONNECTOR_POLL_HPD;
ret = drm_connector_init(encoder->dev, connector,
The patch replaces accesses to display->ctx pointer by container_of construct. It will allow to remove ctx field in the future.
Signed-off-by: Andrzej Hajda a.hajda@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 634904a..abdc3d1 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -304,6 +304,11 @@ struct exynos_dsi { #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host) #define connector_to_dsi(c) container_of(c, struct exynos_dsi, connector)
+static inline struct exynos_dsi *display_to_dsi(struct exynos_drm_display *d) +{ + return container_of(d, struct exynos_dsi, display); +} + static struct exynos_dsi_driver_data exynos3_dsi_driver_data = { .plltmr_reg = 0x50, .has_freqband = 1, @@ -1397,7 +1402,7 @@ static void exynos_dsi_disable(struct exynos_dsi *dsi)
static void exynos_dsi_dpms(struct exynos_drm_display *display, int mode) { - struct exynos_dsi *dsi = display->ctx; + struct exynos_dsi *dsi = display_to_dsi(display);
if (dsi->panel) { switch (mode) { @@ -1486,7 +1491,7 @@ static struct drm_connector_helper_funcs exynos_dsi_connector_helper_funcs = { static int exynos_dsi_create_connector(struct exynos_drm_display *display, struct drm_encoder *encoder) { - struct exynos_dsi *dsi = display->ctx; + struct exynos_dsi *dsi = display_to_dsi(display); struct drm_connector *connector = &dsi->connector; int ret;
@@ -1510,7 +1515,7 @@ static int exynos_dsi_create_connector(struct exynos_drm_display *display, static void exynos_dsi_mode_set(struct exynos_drm_display *display, struct drm_display_mode *mode) { - struct exynos_dsi *dsi = display->ctx; + struct exynos_dsi *dsi = display_to_dsi(display); struct videomode *vm = &dsi->vm;
vm->hactive = mode->hdisplay; @@ -1635,7 +1640,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, void *data) { struct exynos_drm_display *display = dev_get_drvdata(dev); - struct exynos_dsi *dsi = display->ctx; + struct exynos_dsi *dsi = display_to_dsi(display); struct drm_device *drm_dev = data; int ret;
@@ -1653,7 +1658,7 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, void *data) { struct exynos_drm_display *display = dev_get_drvdata(dev); - struct exynos_dsi *dsi = display->ctx; + struct exynos_dsi *dsi = display_to_dsi(display);
exynos_dsi_dpms(display, DRM_MODE_DPMS_OFF);
@@ -1755,8 +1760,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) goto err_del_component; }
- dsi->display.ctx = dsi; - platform_set_drvdata(pdev, &dsi->display);
ret = component_add(dev, &exynos_dsi_component_ops);
On 10/07/2014 02:01 PM, Andrzej Hajda wrote:
Hi Inki,
Many Exynos DRM drivers uses global variables to represent associated devices in Exynos DRM internal framework. It is quite confusing, it adds data duplication and finally it does not allow to handle more than one device in system. It seems better to embed such structures in private context of the device.
Gently ping.
Regards Andrzej
The patchset is based on exynos_drm_next plus my patch [1]: 'drm/exynos: remove explicit encoder/connector de-initialization'.
If the patchset is OK for you I can prepare similar patches for other Exynos DRM components.
Regards Andrzej
Andrzej Hajda (4): drm/exynos/dsi: remove global variable exynos_dsi_display drm/exynos/dsi: simplify device pointer evaluation drm/exynos/dsi: remove redundant encoder field drm/exynos/dsi: stop using display->ctx pointer
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 96 ++++++++++++++++----------------- 1 file changed, 47 insertions(+), 49 deletions(-)
On 2014년 10월 30일 21:39, Andrzej Hajda wrote:
On 10/07/2014 02:01 PM, Andrzej Hajda wrote:
Hi Inki,
Many Exynos DRM drivers uses global variables to represent associated devices in Exynos DRM internal framework. It is quite confusing, it adds data duplication and finally it does not allow to handle more than one device in system. It seems better to embed such structures in private context of the device.
Gently ping.
This patch series already is in exynos-drm-next-todo branch. I will move it to exynos-drm-next after checking.
Thanks, Inki Dae
Regards Andrzej
The patchset is based on exynos_drm_next plus my patch [1]: 'drm/exynos: remove explicit encoder/connector de-initialization'.
If the patchset is OK for you I can prepare similar patches for other Exynos DRM components.
Regards Andrzej
Andrzej Hajda (4): drm/exynos/dsi: remove global variable exynos_dsi_display drm/exynos/dsi: simplify device pointer evaluation drm/exynos/dsi: remove redundant encoder field drm/exynos/dsi: stop using display->ctx pointer
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 96 ++++++++++++++++----------------- 1 file changed, 47 insertions(+), 49 deletions(-)
On 2014년 10월 07일 21:01, Andrzej Hajda wrote:
Hi Inki,
Many Exynos DRM drivers uses global variables to represent associated devices in Exynos DRM internal framework. It is quite confusing, it adds data duplication and finally it does not allow to handle more than one device in system. It seems better to embed such structures in private context of the device.
The patchset is based on exynos_drm_next plus my patch [1]: 'drm/exynos: remove explicit encoder/connector de-initialization'.
If the patchset is OK for you I can prepare similar patches for other Exynos DRM components.
Sorry for late. Applied. Can you prepare similar patches for other? If so, I'd happy.
Thanks, Inki Dae
Regards Andrzej
Andrzej Hajda (4): drm/exynos/dsi: remove global variable exynos_dsi_display drm/exynos/dsi: simplify device pointer evaluation drm/exynos/dsi: remove redundant encoder field drm/exynos/dsi: stop using display->ctx pointer
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 96 ++++++++++++++++----------------- 1 file changed, 47 insertions(+), 49 deletions(-)
On 11/13/2014 09:50 AM, Inki Dae wrote:
On 2014년 10월 07일 21:01, Andrzej Hajda wrote:
Hi Inki,
Many Exynos DRM drivers uses global variables to represent associated devices in Exynos DRM internal framework. It is quite confusing, it adds data duplication and finally it does not allow to handle more than one device in system. It seems better to embed such structures in private context of the device.
The patchset is based on exynos_drm_next plus my patch [1]: 'drm/exynos: remove explicit encoder/connector de-initialization'.
If the patchset is OK for you I can prepare similar patches for other Exynos DRM components.
Sorry for late. Applied. Can you prepare similar patches for other? If so, I'd happy.
Beginning of the next week is OK?
Regards Andrzej
Thanks, Inki Dae
Regards Andrzej
Andrzej Hajda (4): drm/exynos/dsi: remove global variable exynos_dsi_display drm/exynos/dsi: simplify device pointer evaluation drm/exynos/dsi: remove redundant encoder field drm/exynos/dsi: stop using display->ctx pointer
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 96 ++++++++++++++++----------------- 1 file changed, 47 insertions(+), 49 deletions(-)
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Inki Dae