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

Thierry Reding
2012-11-01T20:09:05Z

[-- 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 --]