Re: [PATCH 6/6 V3] hwmon: OMAP4: On die temperature sensor driver

On Thu, 2011-08-25 at 03:24 -0400, Todd Poynor wrote:

On Wed, Aug 24, 2011 at 08:07:12PM +0530, Keerthy wrote:
...

quoted

+	temp_sensor->phy_base = ioremap(mem->start, resource_size(mem));

Check NULL return.

temp_sensor->phy_base is never iounmapped in error paths or _remove
function.

...

quoted

+static int __devexit omap_temp_sensor_remove(struct platform_device *pdev)
+{
+	struct omap_temp_sensor *temp_sensor = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(&pdev->dev);

Should be 
	hwmon_device_unregister(temp_sensor->hwmon_dev);
since that is what was registered, but then the following accesses using
temp_sensor->hwmon_dev won't work anymore and should probably use
&pdev->dev.kobj as parameter instead. Overall the use of
temp_sensor->hwmon_dev vs. &pdev->dev looks questionable to me.

quoted

+	kobject_uevent(&temp_sensor->hwmon_dev->kobj, KOBJ_REMOVE);
+	sysfs_remove_group(&temp_sensor->hwmon_dev->kobj,
+			&omap_temp_sensor_group);
+	omap_temp_sensor_clk_disable(temp_sensor);
+	free_irq(temp_sensor->irq, temp_sensor);

free_irq before omap_temp_sensor_clk_disable, to avoid ISR attempting
to access hardware while unclocked.

Also, t_cold and t_hot interrupts should be disabled before calling
free_irq (and actually before anything is unregistered).

quoted

+	clk_put(temp_sensor->clock);
+	dev_set_drvdata(&pdev->dev, NULL);
+	mutex_destroy(&temp_sensor->sensor_mutex);
+	kfree(temp_sensor);
+
+	return 0;
+}

Todd