All of lore.kernel.org
 help / color / mirror / Atom feed
From: Grant Likely <grant.likely@secretlab.ca>
To: devicetree-discuss@lists.ozlabs.org
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	kernel@pengutronix.de,
	Steffen Trumtrar <s.trumtrar@pengutronix.de>,
	Guennady Liakhovetski <g.liakhovetski@gmx.de>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v10 1/6] video: add display_timing and videomode
Date: Thu, 15 Nov 2012 15:47:53 +0000	[thread overview]
Message-ID: <20121115154753.C82223E194B@localhost> (raw)
In-Reply-To: <1352971437-29877-2-git-send-email-s.trumtrar@pengutronix.de>

On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> Add display_timing structure and the according helper functions. This allows
> the description of a display via its supported timing parameters.
> 
> Every timing parameter can be specified as a single value or a range
> <min typ max>.
> 
> Also, add helper functions to convert from display timings to a generic videomode
> structure. This videomode can then be converted to the corresponding subsystem
> mode representation (e.g. fb_videomode).
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm
making an incorrect assumption.

It looks to me that the purpose of this entire series is to decode video
timings from the device tree and (eventually) provide the data in the
form 'struct videomode'. Correct?

If so, then it looks over engineered. Creating new infrastructure to
allocate, maintain, and free a new 'struct display_timings' doesn't make
any sense when it is an intermediary data format that will never be used
by drivers.

Can the DT parsing code instead return a table of struct videomode?

But, wait... struct videomode is also a new structure. So it looks like
this series creates two new intermediary data structures;
display_timings and videomode. And at least as far as I can see in this
series struct fb_videomode is the only user.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

WARNING: multiple messages have this Message-ID (diff)
From: Grant Likely <grant.likely@secretlab.ca>
To: Steffen Trumtrar <s.trumtrar@pengutronix.de>,
	devicetree-discuss@lists.ozlabs.org
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	kernel@pengutronix.de,
	Steffen Trumtrar <s.trumtrar@pengutronix.de>,
	Guennady Liakhovetski <g.liakhovetski@gmx.de>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v10 1/6] video: add display_timing and videomode
Date: Thu, 15 Nov 2012 15:47:53 +0000	[thread overview]
Message-ID: <20121115154753.C82223E194B@localhost> (raw)
In-Reply-To: <1352971437-29877-2-git-send-email-s.trumtrar@pengutronix.de>

On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote:
> Add display_timing structure and the according helper functions. This allows
> the description of a display via its supported timing parameters.
> 
> Every timing parameter can be specified as a single value or a range
> <min typ max>.
> 
> Also, add helper functions to convert from display timings to a generic videomode
> structure. This videomode can then be converted to the corresponding subsystem
> mode representation (e.g. fb_videomode).
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm
making an incorrect assumption.

It looks to me that the purpose of this entire series is to decode video
timings from the device tree and (eventually) provide the data in the
form 'struct videomode'. Correct?

If so, then it looks over engineered. Creating new infrastructure to
allocate, maintain, and free a new 'struct display_timings' doesn't make
any sense when it is an intermediary data format that will never be used
by drivers.

Can the DT parsing code instead return a table of struct videomode?

But, wait... struct videomode is also a new structure. So it looks like
this series creates two new intermediary data structures;
display_timings and videomode. And at least as far as I can see in this
series struct fb_videomode is the only user.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

  reply	other threads:[~2012-11-15 15:47 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-15  9:23 [PATCH v10 0/6] of: add display helper Steffen Trumtrar
2012-11-15  9:23 ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 1/6] video: add display_timing and videomode Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
2012-11-15 15:47   ` Grant Likely [this message]
2012-11-15 15:47     ` Grant Likely
2012-11-15 16:00     ` Laurent Pinchart
2012-11-15 16:00       ` Laurent Pinchart
2012-11-15 18:03       ` Grant Likely
2012-11-15 18:59         ` Laurent Pinchart
2012-11-15 19:00           ` Laurent Pinchart
2012-11-16  8:53         ` Steffen Trumtrar
2012-11-16  8:53           ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 2/6] video: add of helper for videomode Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 3/6] fbmon: add videomode helpers Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 4/6] fbmon: add of_videomode helpers Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 5/6] drm_modes: add videomode helpers Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
2012-11-15  9:23 ` [PATCH v10 6/6] drm_modes: add of_videomode helpers Steffen Trumtrar
2012-11-15  9:23   ` Steffen Trumtrar
     [not found]   ` <1352971437-29877-7-git-send-email-s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-11-15  9:58     ` Thierry Reding
2012-11-15  9:58       ` Thierry Reding
2012-11-15  9:58       ` Thierry Reding
2012-11-15 10:01       ` Steffen Trumtrar
2012-11-15 10:01         ` Steffen Trumtrar
2012-11-15 10:24 ` [PATCH v10 0/6] of: add display helper Thierry Reding
2012-11-15 10:24   ` Thierry Reding
     [not found]   ` <20121115102411.GA17272-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
2012-11-15 10:27     ` Thierry Reding
2012-11-15 10:27       ` Thierry Reding
2012-11-15 10:27       ` Thierry Reding
2012-11-15 10:28   ` Steffen Trumtrar
2012-11-15 10:28     ` Steffen Trumtrar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121115154753.C82223E194B@localhost \
    --to=grant.likely@secretlab.ca \
    --cc=devicetree-discuss@lists.ozlabs.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=g.liakhovetski@gmx.de \
    --cc=kernel@pengutronix.de \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=s.trumtrar@pengutronix.de \
    --cc=tomi.valkeinen@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.