[PATCH 1/5] DRM: Add i.MX drm core support

[Shawn Guo]

On Thu, Jun 14, 2012 at 03:43:23PM +0200, Sascha Hauer wrote:
...
> +struct drm_device *imx_drm_device_get(void)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_encoder *enc;
> +	struct imx_drm_connector *con;
> +	struct imx_drm_crtc *crtc;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	list_for_each_entry(enc, &imxdrm->encoder_list, list) {
> +		if (!try_module_get(enc->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(enc->owner));
> +			goto unwind_enc;
> +		}
> +	}
> +
> +	list_for_each_entry(con, &imxdrm->connector_list, list) {
> +		if (!try_module_get(con->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(con->owner));
> +			goto unwind_con;
> +		}
> +	}
> +
> +	list_for_each_entry(crtc, &imxdrm->crtc_list, list) {
> +		if (!try_module_get(crtc->owner)) {
> +			dev_err(imxdrm->dev, "could not get module %s\n",
> +					module_name(crtc->owner));
> +			goto unwind_crtc;
> +		}
> +	}
> +
> +	imxdrm->references++;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return imx_drm_device->drm;

Though I'm not quite sure about the point of retrieving the static
variable imx_drm_device from calling __imx_drm_device(), shouldn't
imxdrm be used here since you already retrieve it?

> +
> +unwind_crtc:
> +	list_for_each_entry_continue_reverse(crtc, &imxdrm->crtc_list, list)
> +		module_put(crtc->owner);
> +unwind_con:
> +	list_for_each_entry_continue_reverse(con, &imxdrm->connector_list, list)
> +		module_put(con->owner);
> +unwind_enc:
> +	list_for_each_entry_continue_reverse(enc, &imxdrm->encoder_list, list)
> +		module_put(enc->owner);
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return NULL;
> +
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_device_get);

...

> +/*
> + * register a connector to the drm core
> + */
> +static int imx_drm_connector_register(
> +		struct imx_drm_connector *imx_drm_connector)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	int ret;
> +
> +	drm_connector_init(imxdrm->drm, imx_drm_connector->connector,
> +			imx_drm_connector->connector->funcs,
> +			imx_drm_connector->connector->connector_type);
> +	drm_mode_group_reinit(imxdrm->drm);
> +	ret = drm_sysfs_connector_add(imx_drm_connector->connector);
> +	if (ret)
> +		goto err;

Simply return ret to save the err path?

> +
> +	return 0;
> +err:
> +
> +	return ret;
> +}

...

> +/*
> + * imx_drm_add_encoder - add a new encoder
> + */
> +int imx_drm_add_encoder(struct drm_encoder *encoder,
> +		struct imx_drm_encoder **newenc, struct module *owner)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_encoder *imx_drm_encoder;
> +	int ret;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	if (imxdrm->references) {
> +		ret = -EBUSY;
> +		goto err_busy;
> +	}
> +
> +	imx_drm_encoder = kzalloc(sizeof(struct imx_drm_encoder), GFP_KERNEL);

sizeof(*imx_drm_encoder)

> +	if (!imx_drm_encoder) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	imx_drm_encoder->encoder = encoder;
> +	imx_drm_encoder->owner = owner;
> +
> +	ret = imx_drm_encoder_register(imx_drm_encoder);
> +	if (ret) {
> +		kfree(imx_drm_encoder);
> +		ret = -ENOMEM;
> +		goto err_register;
> +	}
> +
> +	list_add_tail(&imx_drm_encoder->list, &imxdrm->encoder_list);
> +
> +	*newenc = imx_drm_encoder;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return 0;
> +
> +err_register:
> +	kfree(imx_drm_encoder);
> +err_alloc:
> +err_busy:
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_add_encoder);

...

> +/*
> + * imx_drm_add_connector - add a connector
> + */
> +int imx_drm_add_connector(struct drm_connector *connector,
> +		struct imx_drm_connector **new_con,
> +		struct module *owner)
> +{
> +	struct imx_drm_device *imxdrm = __imx_drm_device();
> +	struct imx_drm_connector *imx_drm_connector;
> +	int ret;
> +
> +	mutex_lock(&imxdrm->mutex);
> +
> +	if (imxdrm->references) {
> +		ret = -EBUSY;
> +		goto err_busy;
> +	}
> +
> +	imx_drm_connector = kzalloc(sizeof(struct imx_drm_connector),

sizeof(*imx_drm_connector)

> +			GFP_KERNEL);
> +	if (!imx_drm_connector) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> +
> +	imx_drm_connector->connector = connector;
> +	imx_drm_connector->owner = owner;
> +
> +	ret = imx_drm_connector_register(imx_drm_connector);
> +	if (ret)
> +		goto err_register;
> +
> +	list_add_tail(&imx_drm_connector->list, &imxdrm->connector_list);
> +
> +	*new_con = imx_drm_connector;
> +
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return 0;
> +
> +err_register:
> +	kfree(imx_drm_connector);
> +err_alloc:
> +err_busy:
> +	mutex_unlock(&imxdrm->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_drm_add_connector);

...

> +static int __init imx_drm_init(void)
> +{
> +	int ret;
> +
> +	imx_drm_device = kzalloc(sizeof(*imx_drm_device), GFP_KERNEL);
> +	if (!imx_drm_device)
> +		return -ENOMEM;
> +
> +	mutex_init(&imx_drm_device->mutex);
> +	INIT_LIST_HEAD(&imx_drm_device->crtc_list);
> +	INIT_LIST_HEAD(&imx_drm_device->connector_list);
> +	INIT_LIST_HEAD(&imx_drm_device->encoder_list);
> +
> +	imx_drm_pdev = platform_device_register_simple("imx-drm", -1, NULL, 0);
> +	if (!imx_drm_pdev) {
> +		ret = -EINVAL;
> +		goto err_pdev;
> +	}
> +
> +	imx_drm_pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32),
> +
> +	ret = platform_driver_register(&imx_drm_pdrv);
> +	if (ret)
> +		goto err_pdrv;
> +
> +	return 0;
> +
> +err_pdev:
> +	kfree(imx_drm_device);
> +err_pdrv:
> +	platform_device_unregister(imx_drm_pdev);

The order between these two blocks looks wrong.

> +
> +	return ret;
> +}

...

> +static int __init imx_fb_helper_init(void)
> +{
> +	struct drm_device *drm = imx_drm_device_get();
> +
> +	if (!drm)
> +		return -EINVAL;
> +
> +	fbdev_cma = drm_fbdev_cma_init(drm, PREFERRED_BPP,
> +			drm->mode_config.num_crtc, MAX_CONNECTOR);
> +
> +	imx_drm_fb_helper_set(fbdev_cma);
> +
> +	if (IS_ERR(fbdev_cma)) {
> +		imx_drm_device_put();
> +		return PTR_ERR(fbdev_cma);
> +	}

Shouldn't this error check be put right after drm_fbdev_cma_init call?

> +
> +	return 0;
> +}

-- 
Regards,
Shawn