Re: [PATCH v7 1/8] video: add display_timing struct and helpers

On Thu, Nov 01, 2012 at 09:08:42PM +0100, Thierry Reding wrote:

On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
[...]

quoted

+void timings_release(struct display_timings *disp)
+{
+	int i;
+
+	for (i = 0; i < disp->num_timings; i++)
+		kfree(disp->timings[i]);
+}
+
+void display_timings_release(struct display_timings *disp)
+{
+	timings_release(disp);
+	kfree(disp->timings);
+}

I'm not quite sure I understand how these are supposed to be used. The
only use-case where a struct display_timings is dynamically allocated is
for the OF helpers. In that case, wouldn't it be more useful to have a
function that frees the complete structure, including the struct
display_timings itself? Something like this, which has all of the above
rolled into one:

	void display_timings_free(struct display_timings *disp)
	{
		if (disp->timings) {
			unsigned int i;

			for (i = 0; i < disp->num_timings; i++)
				kfree(disp->timings[i]);
		}

		kfree(disp->timings);
		kfree(disp);
	}

Well, you are right. They can be rolled into one function.
The extra function call is useless and as it seems confusing.

Regards,
Steffen

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |