Re: [PATCH] video: tegra: add tegra display controller and fb driver

On 08/12/2010 11:11 AM, Erik Gilling wrote:

This patch supersedes the previous framebuffer patch

Supports:
	* panel setup
	* overlays
	* suspend / resume

Notable ommisions:
	* support for anything but lvds panels
	* inegration with nvhost driver to sync updates with 3D
	* FB physical geometry is not set
	* lacks interface to set overlay/window x,y offset

Signed-off-by: Erik Gilling <redacted>
Cc: Colin Cross <redacted>
Cc: Travis Geiselbrecht <redacted>

Hi Erik,

Just a couple of notes, not really a full review.

How come this depends on FB=y?

Is that meant to be there?

+struct tegra_dc_blend tegra_dc_blend_modes[][DC_N_WINDOWS] = {
+	{{.nokey = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .one_win = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .two_win_x = BLEND(NOKEY, FIX, 0x00, 0x00),
+	  .two_win_y = BLEND(NOKEY, DEPENDANT, 0x00, 0x00),
+	  .three_win_xy = BLEND(NOKEY, FIX, 0x00, 0x00)},
+	 {.nokey = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .one_win = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .two_win_x = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .two_win_y = BLEND(NOKEY, DEPENDANT, 0x00, 0x00),
+	  .three_win_xy = BLEND(NOKEY, DEPENDANT, 0x00, 0x00)},
+	 {.nokey = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .one_win = BLEND(NOKEY, FIX, 0xff, 0xff),
+	  .two_win_x = BLEND(NOKEY, ALPHA, 0xff, 0xff),
+	  .two_win_y = BLEND(NOKEY, ALPHA, 0xff, 0xff),
+	  .three_win_xy = BLEND(NOKEY, ALPHA, 0xff, 0xff)}
+	}
+};

Tab delimiting this would make it a bit more readable.

+#define DUMP_REG(a) do {			\
+	snprintf(buff, sizeof(buff), "%-32s\t%03x\t%08lx\n", \
+		 #a, a, tegra_dc_readl(dc, a));		      \
+	print(data, buff);				      \
+	} while (0)
+
+static void _dump_regs(struct tegra_dc *dc, void *data,
+		       void (* print)(void *data, const char *str))
+{
+	int i;
+	char buff[256];
+
+	DUMP_REG(DC_CMD_GENERAL_INCR_SYNCPT);
+	DUMP_REG(DC_CMD_GENERAL_INCR_SYNCPT_CNTRL);
+	DUMP_REG(DC_CMD_GENERAL_INCR_SYNCPT_ERROR);

<snip>

The dump_regs code is very long for a debugging feature. Can it just be
replaced by a for loop which prints the offsets and values of each register?

+
+#undef DUMP_REG
+
+#ifdef DEBUG
+static void dump_regs_print(void *data, const char *str)
+{
+	struct tegra_dc *dc = data;
+	dev_dbg(&dc->pdev->dev, "%s", str);
+}
+
+static void dump_regs(struct tegra_dc *dc)
+{
+	_dump_regs(dc, dc, dump_regs_print);
+}
+#else
+
+static void dump_regs(struct tegra_dc *dc) {}
+
+#endif
+
+#ifdef CONFIG_DEBUG_FS
+
+static void dbg_regs_print(void *data, const char *str)
+{
+	struct seq_file *s = data;
+
+	seq_printf(s, "%s", str);
+}
+
+#undef DUMP_REG
+
+static int dbg_dc_show(struct seq_file *s, void *unused)
+{
+	struct tegra_dc *dc = s->private;
+
+	_dump_regs(dc, s, dbg_regs_print);
+
+	return 0;
+}

This all looks a bit confusing (especially the undef stuff). Why do you
have both a debugfs interface to the registers and one which prints them
using dev_dbg?

+static void tegra_dc_dbg_add(struct tegra_dc *dc)
+{
+	char name[32];
+
+	snprintf(name, sizeof(name), "tegra_dc%d_regs", dc->pdev->id);
+	(void) debugfs_create_file(name, S_IRUGO, NULL, dc, &dbg_fops);

Don't cast the return value to void. Possibly print a warning if the
call fails?

+/* does not support syncing windows on multiple dcs in one call */
+int tegra_dc_sync_windows(struct tegra_dc_win *windows[], int n)
+{
+	if (n < 1 || n > DC_N_WINDOWS)
+		return -EINVAL;

 if (n < 1 || n >= DC_N_WINDOWS) ?

+	return wait_event_interruptible_timeout(windows[0]->dc->wq,
+					 tegra_dc_windows_are_clean(windows, n),
+					 HZ);
+}
+EXPORT_SYMBOL(tegra_dc_sync_windows);

<snip>

+static irqreturn_t tegra_dc_irq(int irq, void *ptr)
+{
+	struct tegra_dc *dc = ptr;
+	unsigned long status;
+	unsigned long flags;
+	unsigned long val;
+	int i;
+
+

There are a few places like this with excess blank lines.

+	tegra_dc_init(dc);
+
+	tegra_dc_set_blending(dc, tegra_dc_blend_modes[0]);
+
+	platform_set_drvdata(pdev, dc);
+
+	tegra_dc_dbg_add(dc);
+
+	dev_info(&pdev->dev, "probed\n");

dev_dbg? Also, probably don't need all those blank lines.

+#ifdef CONFIG_PM
+static int tegra_dc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_dc *dc = platform_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "suspend\n");

dev_dbg?

+	disable_irq(dc->irq);
+	tegra_periph_reset_assert(dc->clk);
+	clk_disable(dc->clk);
+
+	return 0;
+}
+
+static int tegra_dc_resume(struct platform_device *pdev)
+{
+	struct tegra_dc *dc = platform_get_drvdata(pdev);
+	struct tegra_dc_win *wins[DC_N_WINDOWS];
+	int i;
+
+	dev_info(&pdev->dev, "resume\n");

dev_dbg?

+extern int suspend_get(char *buffer, struct kernel_param *kp)
+{
+	return 0;
+}
+
+int suspend;

Should be static? Have a global called suspend is likely to cause issues
somewhere.

+module_param_call(suspend, suspend_set, suspend_get, &suspend, 0644);
+
+struct platform_driver tegra_dc_driver = {
+	.driver = {
+		.name = "tegradc",
+		.owner = THIS_MODULE,
+	},
+	.probe = tegra_dc_probe,
+	.remove = tegra_dc_remove,
+#ifdef CONFIG_PM
+	.suspend = tegra_dc_suspend,
+	.resume = tegra_dc_resume,
+#endif
+};

Also should be static?

+static inline unsigned long tegra_dc_readl(struct tegra_dc *dc,
+					   unsigned long reg)
+{
+	return readl(dc->base + reg * 4);
+}

Can these functions use __raw_readl/__raw_writel?

+static void tegra_fb_fillrect(struct fb_info *info,
+			      const struct fb_fillrect *rect)
+{
+	cfb_fillrect(info, rect);
+}
+
+static void tegra_fb_copyarea(struct fb_info *info,
+			      const struct fb_copyarea *region)
+{
+	cfb_copyarea(info, region);
+}
+
+static void tegra_fb_imageblit(struct fb_info *info,
+			       const struct fb_image *image)
+{
+	cfb_imageblit(info, image);
+}

You can just set the cfb_ functions directly in the fb_ops. See below:

+static struct fb_ops tegra_fb_ops = {
+	.owner = THIS_MODULE,
+	.fb_open = tegra_fb_open,
+	.fb_release = tegra_fb_release,
+	.fb_check_var = tegra_fb_check_var,
+	.fb_set_par = tegra_fb_set_par,
+	.fb_setcolreg = tegra_fb_setcolreg,
+	.fb_pan_display = tegra_fb_pan_display,
+	.fb_fillrect = tegra_fb_fillrect,
+	.fb_copyarea = tegra_fb_copyarea,
+	.fb_imageblit = tegra_fb_imageblit,

Should be:

	.fb_fillrect  = cfb_fillrect,
	.fb_copyarea  = cfb_fillarea,
	.fb_imageblit = cfb_imageblit,

Also (nitpicky) tab delimit to make it more readable.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934