On 1/23/23 16:12, Jagan Teki wrote:
> DSI host registration, attach and detach operations are quite
> different for the component and bridge-based DRM drivers.
>
> Supporting generic bridge driver to use both component and bridge
> based DRM drivers can be tricky and would require additional host
> related operation hooks.
>
> Add host operation hooks for registering and unregistering Exynos
> and generic drivers, where Exynos hooks are used in existing Exynos
> component based DRM drivers and generic hooks are used in i.MX8M
> bridge based DRM drivers.
>
> Add host attach and detach operation hooks for Exynos component
> DRM drivers and those get invoked while DSI core host attach and
> detach gets called.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v11:
> - none
> Changes for v10:
> - split from previous series patch
> "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge"
>
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 179 ++++++++++++++++++------
> 1 file changed, 140 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 7afbbe30d1d3..fc7f00ab01b4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -250,6 +250,8 @@ struct exynos_dsi_transfer {
> u16 rx_done;
> };
>
> +struct exynos_dsi;
Is this forward declaration really necessary ? Can't the structures
below be reordered to get rid of this ?
> #define DSIM_STATE_ENABLED BIT(0)
> #define DSIM_STATE_INITIALIZED BIT(1)
> #define DSIM_STATE_CMD_LPM BIT(2)
> @@ -281,12 +283,19 @@ struct exynos_dsi_driver_data {
> const unsigned int *reg_values;
> };
>
> +struct exynos_dsim_host_ops {
> + int (*register_host)(struct exynos_dsi *dsim);
> + void (*unregister_host)(struct exynos_dsi *dsim);
> + int (*attach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
> + int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
> +};
> +
> struct exynos_dsi_plat_data {
> enum exynos_dsi_type hw_type;
> + const struct exynos_dsim_host_ops *host_ops;
> };
>
> struct exynos_dsi {
> - struct drm_encoder encoder;
> struct mipi_dsi_host dsi_host;
> struct drm_bridge bridge;
> struct drm_bridge *out_bridge;
> @@ -316,6 +325,12 @@ struct exynos_dsi {
>
> const struct exynos_dsi_driver_data *driver_data;
> const struct exynos_dsi_plat_data *plat_data;
> +
> + void *priv;
> +};
> +
> +struct exynos_dsi_enc {
> + struct drm_encoder encoder;
> };
>
> #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
> @@ -1319,10 +1334,11 @@ 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 exynos_dsi *dsim = (struct exynos_dsi *)dev_id;
Is the rename really needed ?
> + struct exynos_dsi_enc *dsi = dsim->priv;
Call this variable something else , like dsi_enc , and you shouldn't
need the rename above ...
> struct drm_encoder *encoder = &dsi->encoder;
>
> - if (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE)
> + if (dsim->state & DSIM_STATE_VIDOUT_AVAILABLE)
... and the rename here .
> exynos_drm_crtc_te_handler(encoder->crtc);
>
> return IRQ_HANDLED;
[...]
> static void exynos_dsi_unbind(struct device *dev, struct device *master,
> void *data)
> {
> - struct exynos_dsi *dsi = dev_get_drvdata(dev);
> + struct exynos_dsi *dsim = dev_get_drvdata(dev);
Please avoid the variable renames globally, that should simplify this
patch and remove unrelated changes.
> - exynos_dsi_atomic_disable(&dsi->bridge, NULL);
> + dsim->bridge.funcs->atomic_disable(&dsim->bridge, NULL);
>
> - mipi_dsi_host_unregister(&dsi->dsi_host);
> + mipi_dsi_host_unregister(&dsim->dsi_host);
> }
[...]
With that fixed:
Reviewed-by: Marek Vasut <marex@denx.de>
@@ -250,6 +250,8 @@ struct exynos_dsi_transfer {
u16 rx_done;
};
+struct exynos_dsi;
+
#define DSIM_STATE_ENABLED BIT(0)
#define DSIM_STATE_INITIALIZED BIT(1)
#define DSIM_STATE_CMD_LPM BIT(2)
@@ -281,12 +283,19 @@ struct exynos_dsi_driver_data {
const unsigned int *reg_values;
};
+struct exynos_dsim_host_ops {
+ int (*register_host)(struct exynos_dsi *dsim);
+ void (*unregister_host)(struct exynos_dsi *dsim);
+ int (*attach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
+ int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device);
+};
+
struct exynos_dsi_plat_data {
enum exynos_dsi_type hw_type;
+ const struct exynos_dsim_host_ops *host_ops;
};
struct exynos_dsi {
- struct drm_encoder encoder;
struct mipi_dsi_host dsi_host;
struct drm_bridge bridge;
struct drm_bridge *out_bridge;
@@ -316,6 +325,12 @@ struct exynos_dsi {
const struct exynos_dsi_driver_data *driver_data;
const struct exynos_dsi_plat_data *plat_data;
+
+ void *priv;
+};
+
+struct exynos_dsi_enc {
+ struct drm_encoder encoder;
};
#define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
@@ -1319,10 +1334,11 @@ 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 exynos_dsi *dsim = (struct exynos_dsi *)dev_id;
+ struct exynos_dsi_enc *dsi = dsim->priv;
struct drm_encoder *encoder = &dsi->encoder;
- if (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE)
+ if (dsim->state & DSIM_STATE_VIDOUT_AVAILABLE)
exynos_drm_crtc_te_handler(encoder->crtc);
return IRQ_HANDLED;
@@ -1595,9 +1611,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
struct mipi_dsi_device *device)
{
struct exynos_dsi *dsi = host_to_dsi(host);
+ const struct exynos_dsi_plat_data *pdata = dsi->plat_data;
struct device *dev = dsi->dev;
- struct drm_encoder *encoder = &dsi->encoder;
- struct drm_device *drm = encoder->dev;
int ret;
dsi->out_bridge = devm_drm_of_dsi_get_bridge(dev, dev->of_node, 1, 0);
@@ -1611,35 +1626,15 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
drm_bridge_add(&dsi->bridge);
- drm_bridge_attach(encoder, &dsi->bridge,
- list_first_entry_or_null(&encoder->bridge_chain,
- struct drm_bridge,
- chain_node), 0);
-
- /*
- * This is a temporary solution and should be made by more generic way.
- *
- * If attached panel device is for command mode one, dsi should register
- * TE interrupt handler.
- */
- if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
- ret = exynos_dsi_register_te_irq(dsi, &device->dev);
- if (ret)
+ if (pdata->host_ops && pdata->host_ops->attach) {
+ ret = pdata->host_ops->attach(dsi, device);
+ if (ret < 0)
return ret;
}
- mutex_lock(&drm->mode_config.mutex);
-
dsi->lanes = device->lanes;
dsi->format = device->format;
dsi->mode_flags = device->mode_flags;
- exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
- !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
-
- mutex_unlock(&drm->mode_config.mutex);
-
- if (drm->mode_config.poll_enabled)
- drm_kms_helper_hotplug_event(drm);
return 0;
}
@@ -1648,12 +1643,14 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
struct mipi_dsi_device *device)
{
struct exynos_dsi *dsi = host_to_dsi(host);
- struct drm_device *drm = dsi->encoder.dev;
-
- if (drm->mode_config.poll_enabled)
- drm_kms_helper_hotplug_event(drm);
+ const struct exynos_dsi_plat_data *pdata = dsi->plat_data;
+ int ret;
- exynos_dsi_unregister_te_irq(dsi);
+ if (pdata->host_ops && pdata->host_ops->detach) {
+ ret = pdata->host_ops->detach(dsi, device);
+ if (ret < 0)
+ return ret;
+ }
drm_bridge_remove(&dsi->bridge);
@@ -1727,10 +1724,66 @@ static int exynos_dsi_parse_dt(struct exynos_dsi *dsi)
return 0;
}
+static int _exynos_dsi_host_attach(struct exynos_dsi *dsim,
+ struct mipi_dsi_device *device)
+{
+ struct exynos_dsi_enc *dsi = dsim->priv;
+ struct drm_encoder *encoder = &dsi->encoder;
+ struct drm_device *drm = encoder->dev;
+ int ret;
+
+ drm_bridge_attach(encoder, &dsim->bridge,
+ list_first_entry_or_null(&encoder->bridge_chain,
+ struct drm_bridge,
+ chain_node), 0);
+
+ /*
+ * This is a temporary solution and should be made by more generic way.
+ *
+ * If attached panel device is for command mode one, dsi should register
+ * TE interrupt handler.
+ */
+ if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+ ret = exynos_dsi_register_te_irq(dsim, &device->dev);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(&drm->mode_config.mutex);
+
+ dsim->lanes = device->lanes;
+ dsim->format = device->format;
+ dsim->mode_flags = device->mode_flags;
+ exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
+ !(dsim->mode_flags & MIPI_DSI_MODE_VIDEO);
+
+ mutex_unlock(&drm->mode_config.mutex);
+
+ if (drm->mode_config.poll_enabled)
+ drm_kms_helper_hotplug_event(drm);
+
+ return 0;
+}
+
+static int _exynos_dsi_host_detach(struct exynos_dsi *dsim,
+ struct mipi_dsi_device *device)
+{
+ struct exynos_dsi_enc *dsi = dsim->priv;
+ struct drm_device *drm = dsi->encoder.dev;
+
+ if (drm->mode_config.poll_enabled)
+ drm_kms_helper_hotplug_event(drm);
+
+ exynos_dsi_unregister_te_irq(dsim);
+
+ return 0;
+}
+
static int exynos_dsi_bind(struct device *dev, struct device *master,
void *data)
{
- struct exynos_dsi *dsi = dev_get_drvdata(dev);
+ struct exynos_dsi *dsim = dev_get_drvdata(dev);
+ struct exynos_dsi_enc *dsi = dsim->priv;
struct drm_encoder *encoder = &dsi->encoder;
struct drm_device *drm_dev = data;
int ret;
@@ -1741,17 +1794,17 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
if (ret < 0)
return ret;
- return mipi_dsi_host_register(&dsi->dsi_host);
+ return mipi_dsi_host_register(&dsim->dsi_host);
}
static void exynos_dsi_unbind(struct device *dev, struct device *master,
void *data)
{
- struct exynos_dsi *dsi = dev_get_drvdata(dev);
+ struct exynos_dsi *dsim = dev_get_drvdata(dev);
- exynos_dsi_atomic_disable(&dsi->bridge, NULL);
+ dsim->bridge.funcs->atomic_disable(&dsim->bridge, NULL);
- mipi_dsi_host_unregister(&dsi->dsi_host);
+ mipi_dsi_host_unregister(&dsim->dsi_host);
}
static const struct component_ops exynos_dsi_component_ops = {
@@ -1759,6 +1812,40 @@ static const struct component_ops exynos_dsi_component_ops = {
.unbind = exynos_dsi_unbind,
};
+static int exynos_dsi_register_host(struct exynos_dsi *dsim)
+{
+ struct exynos_dsi_enc *dsi;
+
+ dsi = devm_kzalloc(dsim->dev, sizeof(*dsi), GFP_KERNEL);
+ if (!dsi)
+ return -ENOMEM;
+
+ dsim->priv = dsi;
+ dsim->bridge.pre_enable_prev_first = true;
+
+ return component_add(dsim->dev, &exynos_dsi_component_ops);
+}
+
+static void exynos_dsi_unregister_host(struct exynos_dsi *dsim)
+{
+ component_del(dsim->dev, &exynos_dsi_component_ops);
+}
+
+static int generic_dsim_register_host(struct exynos_dsi *dsim)
+{
+ return mipi_dsi_host_register(&dsim->dsi_host);
+}
+
+static void generic_dsim_unregister_host(struct exynos_dsi *dsim)
+{
+ mipi_dsi_host_unregister(&dsim->dsi_host);
+}
+
+static const struct exynos_dsim_host_ops generic_dsim_host_ops = {
+ .register_host = generic_dsim_register_host,
+ .unregister_host = generic_dsim_unregister_host,
+};
+
static const struct drm_bridge_timings dsim_bridge_timings_de_low = {
.input_bus_flags = DRM_BUS_FLAG_DE_LOW,
};
@@ -1853,7 +1940,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM)
dsi->bridge.timings = &dsim_bridge_timings_de_low;
- ret = component_add(dev, &exynos_dsi_component_ops);
+ if (dsi->plat_data->host_ops && dsi->plat_data->host_ops->register_host)
+ ret = dsi->plat_data->host_ops->register_host(dsi);
+
if (ret)
goto err_disable_runtime;
@@ -1944,24 +2033,36 @@ static const struct dev_pm_ops exynos_dsi_pm_ops = {
pm_runtime_force_resume)
};
+static const struct exynos_dsim_host_ops exynos_dsi_host_ops = {
+ .register_host = exynos_dsi_register_host,
+ .unregister_host = exynos_dsi_unregister_host,
+ .attach = _exynos_dsi_host_attach,
+ .detach = _exynos_dsi_host_detach,
+};
+
static const struct exynos_dsi_plat_data exynos3250_dsi_pdata = {
.hw_type = DSIM_TYPE_EXYNOS3250,
+ .host_ops = &exynos_dsi_host_ops,
};
static const struct exynos_dsi_plat_data exynos4210_dsi_pdata = {
.hw_type = DSIM_TYPE_EXYNOS4210,
+ .host_ops = &exynos_dsi_host_ops,
};
static const struct exynos_dsi_plat_data exynos5410_dsi_pdata = {
.hw_type = DSIM_TYPE_EXYNOS5410,
+ .host_ops = &exynos_dsi_host_ops,
};
static const struct exynos_dsi_plat_data exynos5422_dsi_pdata = {
.hw_type = DSIM_TYPE_EXYNOS5422,
+ .host_ops = &exynos_dsi_host_ops,
};
static const struct exynos_dsi_plat_data exynos5433_dsi_pdata = {
.hw_type = DSIM_TYPE_EXYNOS5433,
+ .host_ops = &exynos_dsi_host_ops,
};
static const struct of_device_id exynos_dsi_of_match[] = {