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

[Michael Tretter <m.tretter@pengutronix.de>]

On Tue, 15 Sep 2020 19:07:59 +0200, Andrzej Hajda wrote:
> 
> 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.

Correct, but if the driver is used as a drm bridge, there won't be an encoder
until bridge_attach. Mipi dsi devices defer their probe until the mipi dsi
host is available. If I move the mipi dsi host registration into
bridge_attach, the driver does not know if the next device is another bridge
or a panel during bridge_attach, but the driver cannot successfully return
from bridge_attach, because then the drm driver expects a connector.

I guess that the encoder should be initialized before registering the mipi dsi
host during probe instead of bind. The probe won't be rolled back on
PROBE_DEFER during bind and the encoder will be available in host_attach.
When splitting the driver into the exynos platform specific code and the more
generic code, there won't be an encoder during host_attach in the generic
code, but the host_ops callback could (and will) use the platform specific
encoder, which is available before bridge_attach.

Does this make sense to you?

Michael

> 
> 
> 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);
> >   
>