On Thu, 30 Jan 2014 19:58:43 -0200
Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > + seq_printf(m, ", mode:\n");
> > + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x\n",
> > + mode->base.id, mode->name,
> > + mode->vrefresh, mode->clock,
> > + mode->hdisplay, mode->hsync_start,
> > + mode->hsync_end, mode->htotal,
> > + mode->vdisplay, mode->vsync_start,
> > + mode->vsync_end, mode->vtotal,
> > + mode->type, mode->flags);
> > + } else {
> > + seq_printf(m, "\n");
>
> for cases like this shouldn't we use seq_put instead of seq_printf?
Yeah I guess that's a bit simpler, I'll switch it over.
> > + seq_printf(m, "\tfixed mode:\n");
> > + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x\n",
> > + mode->base.id, mode->name,
> > + mode->vrefresh, mode->clock,
> > + mode->hdisplay, mode->hsync_start,
> > + mode->hsync_end, mode->htotal,
> > + mode->vdisplay, mode->vsync_start,
> > + mode->vsync_end, mode->vtotal,
> > + mode->type, mode->flags);
> > +}
>
> I would prefer a more bloated info, with variable_name =
> vriable_value... I know this is bloated, but I'm also think on our
> lazyness when reading bug reports ;)
Yeah I always have to look up the field names too, I'll annotate this
just to make things extra easy for us. :)
> > +#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
> > + list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
> > + if ((intel_connector)->base.encoder == (__encoder))
>
> are all of this parenthesis needed?
I think that's the minimal amount, yeah. Anything passed in to the
macro needs to have parens around it just in case it's an expression
that would cause trouble when expanded into the code.
Thanks for the review, I'll re-post shortly.
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
