On 11/03/16 20:15, Laurent Pinchart wrote:
> Hi Jyri,
>
> Thank you for the patch.
>
> On Wednesday 02 Nov 2016 17:57:50 Jyri Sarha wrote:
>> Stop using struct drm_driver load() and unload() callbacks. The
>> callbacks should not be used anymore. Instead of using load the
>> drm_device is allocated with drm_dev_alloc() and registered with
>> drm_dev_register() only after the driver is completely initialized.
>> The deinitialization is done directly either in component unbind
>> callback or in platform driver demove callback.
>>
>> Signed-off-by: Jyri Sarha <jsarha at ti.com>
>> ---
>> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 124 ++++++++++++++++++--------------
>> 1 file changed, 70 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 5cf534c..11f3262 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -194,18 +194,22 @@ static int cpufreq_transition(struct notifier_block
>> *nb, * DRM operations:
>> */
>>
>> -static int tilcdc_unload(struct drm_device *dev)
>> +static void tilcdc_fini(struct drm_device *dev)
>> {
>> struct tilcdc_drm_private *priv = dev->dev_private;
>>
>> - tilcdc_remove_external_encoders(dev);
>> + drm_modeset_lock_crtc(priv->crtc, NULL);
>> + tilcdc_crtc_disable(priv->crtc);
>> + drm_modeset_unlock_crtc(priv->crtc);
>> +
>> + drm_dev_unregister(dev);
>>
>> - drm_fbdev_cma_fini(priv->fbdev);
>> drm_kms_helper_poll_fini(dev);
>> + drm_fbdev_cma_fini(priv->fbdev);
>
> Is there a need to reorder these ?
I am not sure actually. When the things did not work properly in the
beginning I just ordered unloading to happen strictly in the reverse
order compared to loading. I did not go back to check what changes were
actually needed when I got things working.
>
>> + drm_irq_uninstall(dev);
>> drm_mode_config_cleanup(dev);
>> - drm_vblank_cleanup(dev);
>>
>> - drm_irq_uninstall(dev);
>> + tilcdc_remove_external_encoders(dev);
>>
>> #ifdef CONFIG_CPU_FREQ
>> cpufreq_unregister_notifier(&priv->freq_transition,
>> @@ -225,28 +229,34 @@ static int tilcdc_unload(struct drm_device *dev)
>>
>> pm_runtime_disable(dev->dev);
>>
>> - return 0;
>> + drm_dev_unref(dev);
>> }
>>
>> -static int tilcdc_load(struct drm_device *dev, unsigned long flags)
>> +static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
>> {
>> - struct platform_device *pdev = dev->platformdev;
>> - struct device_node *node = pdev->dev.of_node;
>> + struct drm_device *ddev;
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct device_node *node = dev->of_node;
>> struct tilcdc_drm_private *priv;
>> struct resource *res;
>> u32 bpp = 0;
>> int ret;
>>
>> - priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> if (!priv) {
>> - dev_err(dev->dev, "failed to allocate private data\n");
>> + dev_err(dev, "failed to allocate private data\n");
>> return -ENOMEM;
>> }
>>
>> - dev->dev_private = priv;
>> + ddev = drm_dev_alloc(ddrv, dev);
>
> As a follow-up patch you might want to move tilcdc_driver above this function
> and use it directly to remove the ddrv argument.
>
Ok, thanks.
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + if (IS_ERR(ddev))
>> + return PTR_ERR(ddev);
>> +
>> + ddev->platformdev = pdev;
>> + ddev->dev_private = priv;
>
> [snip]
>
