Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Xin Ji <xji@analogixsemi.com>
Cc: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Nicolas Boichat <drinkcat@chromium.org>,
	Jonas Karlman <jonas@kwiboo.se>, David Airlie <airlied@linux.ie>,
	Neil Armstrong <narmstrong@baylibre.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Sheng Pan <span@analogixsemi.com>
Subject: Re: [PATCH v2 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver
Date: Wed, 9 Oct 2019 16:06:45 +0300	[thread overview]
Message-ID: <20191009130645.GN25098@kadam> (raw)
In-Reply-To: <20191009113032.GL25098@kadam>

On Wed, Oct 09, 2019 at 02:30:32PM +0300, Dan Carpenter wrote:
> > +	platform = kzalloc(sizeof(*platform), GFP_KERNEL);
> > +	if (!platform) {
> > +		DRM_DEV_ERROR(dev, "failed to allocate driver data\n");
> > +		ret = -ENOMEM;
> > +		goto exit;
> 
> return -ENOMEM;
> 
> > +	}
> > +
> > +	pdata = &platform->pdata;
> > +
> > +	/* device tree parsing function call */
> > +	ret = anx7625_parse_dt(&client->dev, pdata);
> > +	if (ret != 0)	/* if occurs error */
> > +		goto err0;
> 
> != 0 is double negative.  Choose better label names.  Remove the obvious
> comment.
> 
> if (ret)
> 	goto free_platform;
> 
> > +
> > +	anx7625_init_gpio(platform);
> > +
> > +	/* to access global platform data */
> > +	platform->client = client;
> > +	i2c_set_clientdata(client, platform);
> > +
> > +	if (platform->pdata.extcon_supported) {
> > +		/* get extcon device from DTS */
> > +		platform->extcon = extcon_get_edev_by_phandle(&client->dev, 0);
> > +		if (PTR_ERR(platform->extcon) == -EPROBE_DEFER)
> > +			goto err0;
> 
> Preserve the error code.
> 
> > +		if (IS_ERR(platform->extcon)) {
> > +			DRM_DEV_ERROR(&client->dev,
> > +				      "can not get extcon device!");
> > +			goto err0;
> 
> Prerve the error code.
> 
> > +		}
> > +
> > +		ret = anx7625_extcon_notifier_init(platform);
> > +		if (ret < 0)
> > +			goto err0;
> > +	}
> > +
> > +	atomic_set(&platform->power_status, 0);
> > +
> > +	mutex_init(&platform->lock);
> > +
> > +	if (platform->pdata.gpio_intr_hpd) {
> > +		INIT_WORK(&platform->work, anx7625_work_func);
> > +		platform->workqueue = create_workqueue("anx7625_work");
> > +		if (!platform->workqueue) {
> > +			DRM_DEV_ERROR(dev, "failed to create work queue\n");
> > +			ret = -ENOMEM;
> > +			goto err1;
> 
> This goto will crash.  Because you have forgotten what the most recently
> allocated resource was.  It should be "goto free_platform;" still.
> 
> > +		}
> > +
> > +		platform->pdata.hpd_irq =
> > +			gpiod_to_irq(platform->pdata.gpio_intr_hpd);
> > +		if (platform->pdata.hpd_irq < 0) {
> > +			DRM_DEV_ERROR(dev, "failed to get gpio irq\n");
> > +			goto err1;
> 
> goto free_wq;
> 
> > +		}
> > +
> > +		ret = request_threaded_irq(platform->pdata.hpd_irq,
> > +					   NULL, anx7625_intr_hpd_isr,
> > +					   IRQF_TRIGGER_FALLING |
> > +					   IRQF_TRIGGER_RISING |
> > +					   IRQF_ONESHOT,
> > +					   "anx7625-hpd", platform);
> > +		if (ret < 0) {
> > +			DRM_DEV_ERROR(dev, "failed to request irq\n");
> > +			goto err1;
> > +		}
> > +
> > +		ret = irq_set_irq_wake(platform->pdata.hpd_irq, 1);
> > +		if (ret < 0) {
> > +			DRM_DEV_ERROR(dev, "Request irq for hpd");
> > +			DRM_DEV_ERROR(dev, "interrupt wake set fail\n");
> > +			goto err1;
> > +		}
> > +
> > +		ret = enable_irq_wake(platform->pdata.hpd_irq);
> > +		if (ret < 0) {
> > +			DRM_DEV_ERROR(dev, "Enable irq for HPD");
> > +			DRM_DEV_ERROR(dev, "interrupt wake enable fail\n");
> > +			goto err1;
> > +		}
> > +	}
> > +
> > +	if (anx7625_register_i2c_dummy_clients(platform, client) != 0) {
> 
> Preserve the error code.
> 
> 	ret = anx7625_register_i2c_dummy_clients();
> 	if (ret)
> 		goto free_platform;
> 

I meant goto free_wq here.  That's the most recent allocation.

regards,
dan carpenter