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

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