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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
