Re: [PATCH v2 08/16] drm/exynos: add host_ops callback for platform drivers

[Andrzej Hajda <a.hajda@samsung.com>]

W dniu 11.09.2020 o 15:54, Michael Tretter pisze:
> Platform drivers need to be aware if a mipi dsi device attaches or
> detaches. Add host_ops to the driver_data for attach and detach
> callbacks and move the i80 mode selection and the hotplug handling into
> the callback, because these depend on the drm driver.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> v2:
> - new patch
> ---
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 64 ++++++++++++++++++++-----
>   1 file changed, 53 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 1a15ae71205d..684a2fbef08a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -239,6 +239,12 @@ struct exynos_dsi_transfer {
>   #define DSIM_STATE_CMD_LPM		BIT(2)
>   #define DSIM_STATE_VIDOUT_AVAILABLE	BIT(3)
>   
> +struct exynos_dsi;
> +struct exynos_dsi_host_ops {
> +	int (*attach)(struct device *dev, struct mipi_dsi_device *device);
> +	int (*detach)(struct device *dev, struct mipi_dsi_device *device);
> +};
> +
>   enum exynos_reg_offset {
>   	EXYNOS_REG_OFS,
>   	EXYNOS5433_REG_OFS
> @@ -254,6 +260,7 @@ struct exynos_dsi_driver_data {
>   	unsigned int wait_for_reset;
>   	unsigned int num_bits_resol;
>   	const unsigned int *reg_values;
> +	const struct exynos_dsi_host_ops *host_ops;
>   };
>   
>   struct exynos_dsi {
> @@ -467,6 +474,41 @@ static const unsigned int exynos5433_reg_values[] = {
>   	[PHYTIMING_HS_TRAIL] = 0x0c,
>   };
>   
> +static int __exynos_dsi_host_attach(struct device *dev,
> +				    struct mipi_dsi_device *device)
> +{
> +	struct exynos_dsi *dsi = dev_get_drvdata(dev);
> +	struct drm_device *drm = dsi->encoder.dev;


As I wrote yesterday drm device was guaranteed to be present here only 
if mipi dsi host registration was performed in component bind. In case 
it is performed in probe it will be always NULL, and the code does not 
make sense.


Regards

Andrzej



> +	struct exynos_drm_crtc *crtc;
> +
> +	mutex_lock(&drm->mode_config.mutex);
> +	crtc = exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD);
> +	crtc->i80_mode = !(device->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 device *dev,
> +				     struct mipi_dsi_device *device)
> +{
> +	struct exynos_dsi *dsi = dev_get_drvdata(dev);
> +	struct drm_device *drm = dsi->encoder.dev;
> +
> +	if (drm->mode_config.poll_enabled)
> +		drm_kms_helper_hotplug_event(drm);
> +
> +	return 0;
> +}
> +
> +static const struct exynos_dsi_host_ops exynos_dsi_host_ops = {
> +	.attach = __exynos_dsi_host_attach,
> +	.detach = __exynos_dsi_host_detach,
> +};
> +
>   static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
>   	.reg_ofs = EXYNOS_REG_OFS,
>   	.plltmr_reg = 0x50,
> @@ -477,6 +519,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
>   	.wait_for_reset = 1,
>   	.num_bits_resol = 11,
>   	.reg_values = reg_values,
> +	.host_ops = &exynos_dsi_host_ops,
>   };
>   
>   static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
> @@ -489,6 +532,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
>   	.wait_for_reset = 1,
>   	.num_bits_resol = 11,
>   	.reg_values = reg_values,
> +	.host_ops = &exynos_dsi_host_ops,
>   };
>   
>   static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
> @@ -499,6 +543,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
>   	.wait_for_reset = 1,
>   	.num_bits_resol = 11,
>   	.reg_values = reg_values,
> +	.host_ops = &exynos_dsi_host_ops,
>   };
>   
>   static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
> @@ -510,6 +555,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
>   	.wait_for_reset = 0,
>   	.num_bits_resol = 12,
>   	.reg_values = exynos5433_reg_values,
> +	.host_ops = &exynos_dsi_host_ops,
>   };
>   
>   static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
> @@ -521,6 +567,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
>   	.wait_for_reset = 1,
>   	.num_bits_resol = 12,
>   	.reg_values = exynos5422_reg_values,
> +	.host_ops = &exynos_dsi_host_ops,
>   };
>   
>   static const struct of_device_id exynos_dsi_of_match[] = {
> @@ -1551,8 +1598,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_host_ops *ops = dsi->driver_data->host_ops;
>   	struct drm_encoder *encoder = &dsi->encoder;
> -	struct drm_device *drm = encoder->dev;
>   	struct drm_bridge *out_bridge;
>   
>   	out_bridge  = of_drm_find_bridge(device->dev.of_node);
> @@ -1590,18 +1637,12 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>   			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);
> +	if (ops && ops->attach)
> +		ops->attach(dsi->dsi_host.dev, device);
>   
>   	return 0;
>   }
> @@ -1610,6 +1651,7 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>   				  struct mipi_dsi_device *device)
>   {
>   	struct exynos_dsi *dsi = host_to_dsi(host);
> +	const struct exynos_dsi_host_ops *ops = dsi->driver_data->host_ops;
>   	struct drm_device *drm = dsi->encoder.dev;
>   
>   	if (dsi->panel) {
> @@ -1625,8 +1667,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>   		INIT_LIST_HEAD(&dsi->bridge_chain);
>   	}
>   
> -	if (drm->mode_config.poll_enabled)
> -		drm_kms_helper_hotplug_event(drm);
> +	if (ops && ops->detach)
> +		ops->detach(dsi->dsi_host.dev, device);
>   
>   	exynos_dsi_unregister_te_irq(dsi);
>