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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote:
[...]
> +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);
	}

Is there a use-case where a struct display_timings is not dynamically
allocated? The only one I can think of is where it is defined as
platform data, but in that case you don't want to be calling
display_timing_release() on it anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]