[PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[Xiubo Li]

Some Freescale SoCs, there has an DVI/HDMI controller and a PHY,
attached to one of their display controller unit's LCDC interfaces.
This patch adds a preliminary static support for such controllers.

This will support for many modes and a dynamic switching between
them.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../devicetree/bindings/video/fsl-sii902x.txt      |  17 +
 drivers/video/fbdev/Kconfig                        |   7 +
 drivers/video/fbdev/Makefile                       |   1 +
 drivers/video/fbdev/fsl-sii902x.c                  | 526 +++++++++++++++++++++
 4 files changed, 551 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/fsl-sii902x.txt
 create mode 100644 drivers/video/fbdev/fsl-sii902x.c

diff --git a/Documentation/devicetree/bindings/video/fsl-sii902x.txt b/Documentation/devicetree/bindings/video/fsl-sii902x.txt
new file mode 100644
index 0000000..55c63ad6
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fsl-sii902x.txt
@@ -0,0 +1,17 @@
+Device-Tree binding for SII902X HDMI driver
+
+Required properties:
+- compatible:		Must contain "fsl,sii902x".
+- reg:			I2C address of the SII902X HDMI device.
+- interrupts:		One interrupt of the fb dev.
+
+Example:
+&i2c1 {
+        status = "okay";
+
+        hdmi: sii9022a@39 {
+              compatible = "fsl,sii902x";
+              reg = <0x39>;
+              interrupts = <GIC_SPI 167 IRQ_TYPE_EDGE_RISING>;
+        };
+};
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index ccbe2ae..946eb5c 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -1976,6 +1976,13 @@ config FB_FSL_DIU
 	---help---
 	  Framebuffer driver for the Freescale SoC DIU
 
+config FB_FSL_SII902X
+        tristate "Si Image SII902X DVI/HDMI Interface Chip"
+	depends on FB && FSL_SOC
+	select FB_MODE_HELPERS
+	---help---
+	  Driver for the Freescale SoC's DVI/HDMI controller.
+
 config FB_W100
 	tristate "W100 frame buffer support"
 	depends on FB && ARCH_PXA
diff --git a/drivers/video/fbdev/Makefile b/drivers/video/fbdev/Makefile
index 1979aff..9aeb441 100644
--- a/drivers/video/fbdev/Makefile
+++ b/drivers/video/fbdev/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_FB_IMX)              += imxfb.o
 obj-$(CONFIG_FB_S3C)		  += s3c-fb.o
 obj-$(CONFIG_FB_S3C2410)	  += s3c2410fb.o
 obj-$(CONFIG_FB_FSL_DIU)	  += fsl-diu-fb.o
+obj-$(CONFIG_FB_FSL_SII902X)      += fsl-sii902x.o
 obj-$(CONFIG_FB_COBALT)           += cobalt_lcdfb.o
 obj-$(CONFIG_FB_IBM_GXT4500)	  += gxt4500.o
 obj-$(CONFIG_FB_PS3)		  += ps3fb.o
diff --git a/drivers/video/fbdev/fsl-sii902x.c b/drivers/video/fbdev/fsl-sii902x.c
new file mode 100644
index 0000000..e0fb3dd
--- /dev/null
+++ b/drivers/video/fbdev/fsl-sii902x.c
@@ -0,0 +1,526 @@
+/*
+ * Copyright (C) 2010-2014 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/fsl_devices.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include "edid.h"
+
+static bool g_enable_hdmi;
+
+struct sii902x_data {
+	struct i2c_client *client;
+	struct delayed_work det_work;
+	struct fb_info *fbi;
+	struct regmap *regmap;
+	unsigned int irq;
+	u8 cable_plugin;
+} *sii902x;
+
+static struct i2c_client *sii902x_to_i2c(struct sii902x_data *sii902x)
+{
+	return sii902x->client;
+}
+
+static s32 sii902x_write(const struct i2c_client *client,
+			u8 command, u8 value)
+{
+	return i2c_smbus_write_byte_data(client, command, value);
+}
+
+static s32 sii902x_read(const struct i2c_client *client, u8 command)
+{
+	int val;
+
+	val = i2c_smbus_read_word_data(client, command);
+
+	return val & 0xff;
+}
+
+static ssize_t sii902x_show_name(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	strcpy(buf, sii902x->fbi->fix.id);
+	sprintf(buf+strlen(buf), "\n");
+
+	return strlen(buf);
+}
+
+static DEVICE_ATTR(fb_name, S_IRUGO, sii902x_show_name, NULL);
+
+static ssize_t sii902x_show_state(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	if (sii902x->cable_plugin == 0)
+		strcpy(buf, "plugout\n");
+	else
+		strcpy(buf, "plugin\n");
+
+	return strlen(buf);
+}
+
+static DEVICE_ATTR(cable_state, S_IRUGO, sii902x_show_state, NULL);
+
+static void sii902x_power_up_tx(struct sii902x_data *sii902x)
+{
+	struct i2c_client *client = sii902x_to_i2c(sii902x);
+	int val;
+
+	val = sii902x_read(client, 0x1E);
+	val &= ~0x3;
+	sii902x_write(client, 0x1E, val);
+}
+
+static void sii902x_setup(struct fb_info *fbi)
+{
+	u16 data[4];
+	u32 refresh;
+	u8 *tmp;
+	int i;
+
+	/* Power up */
+	sii902x_power_up_tx(sii902x);
+
+	/* set TPI video mode */
+	data[0] = PICOS2KHZ(fbi->var.pixclock) / 10;
+	data[2] = fbi->var.hsync_len + fbi->var.left_margin +
+		  fbi->var.xres + fbi->var.right_margin;
+	data[3] = fbi->var.vsync_len + fbi->var.upper_margin +
+		  fbi->var.yres + fbi->var.lower_margin;
+	refresh = data[2] * data[3];
+	refresh = (PICOS2KHZ(fbi->var.pixclock) * 1000) / refresh;
+	data[1] = refresh * 100;
+	tmp = (u8 *)data;
+	for (i = 0; i < 8; i++)
+		sii902x_write(sii902x->client, i, tmp[i]);
+
+	/* input bus/pixel: full pixel wide (24bit), rising edge */
+	sii902x_write(sii902x->client, 0x08, 0x70);
+	/* Set input format to RGB */
+	sii902x_write(sii902x->client, 0x09, 0x00);
+	/* set output format to RGB */
+	sii902x_write(sii902x->client, 0x0A, 0x00);
+	/* audio setup */
+	sii902x_write(sii902x->client, 0x25, 0x00);
+	sii902x_write(sii902x->client, 0x26, 0x40);
+	sii902x_write(sii902x->client, 0x27, 0x00);
+}
+
+static int __sii902x_read_edid(struct i2c_adapter *adp,
+			unsigned char *edid, u8 *buf)
+{
+	unsigned short addr = 0x50;
+	int ret;
+
+	struct i2c_msg msg[2] = {
+		{
+		.addr	= addr,
+		.flags	= 0,
+		.len	= 1,
+		.buf	= buf,
+		}, {
+		.addr	= addr,
+		.flags	= I2C_M_RD,
+		.len	= EDID_LENGTH,
+		.buf	= edid,
+		},
+	};
+
+	if (adp == NULL)
+		return -EINVAL;
+
+	memset(edid, 0, EDID_LENGTH);
+
+	ret = i2c_transfer(adp, msg, 2);
+	if (ret < 0)
+		return ret;
+
+	/* If 0x50 fails, try 0x37. */
+	if (edid[1] == 0x00) {
+		msg[0].addr = msg[1].addr = 0x37;
+		ret = i2c_transfer(adp, msg, 2);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (edid[1] == 0x00)
+		return -ENOENT;
+
+	return 0;
+}
+/* make sure edid has 256 bytes*/
+static int __sii902x_get_edid(struct i2c_adapter *adp,
+			      struct fb_info *fbi)
+{
+	u8 *edid;
+	u8 buf[2] = {0, 0};
+	int num, ret;
+
+	edid = kzalloc(EDID_LENGTH, GFP_KERNEL);
+	if (!edid)
+		return -ENOMEM;
+
+	ret = __sii902x_read_edid(adp, edid, buf);
+	if (ret)
+		return ret;
+
+	/* edid first block parsing */
+	memset(&fbi->monspecs, 0, sizeof(fbi->monspecs));
+	fb_edid_to_monspecs(edid, &fbi->monspecs);
+
+	/* need read ext block? Only support one more blk now */
+	num = edid[0x7E];
+	if (num) {
+		buf[0] = 0x80;
+		ret = __sii902x_read_edid(adp, edid, buf);
+		if (ret)
+			return ret;
+
+		fb_edid_add_monspecs(edid, &fbi->monspecs);
+	}
+
+	kfree(edid);
+	return 0;
+}
+static int sii902x_get_edid(struct fb_info *fbi)
+{
+	int old, dat, ret, cnt = 100;
+
+	old = sii902x_read(sii902x->client, 0x1A);
+
+	sii902x_write(sii902x->client, 0x1A, old | 0x4);
+	do {
+		cnt--;
+		msleep(20);
+		dat = sii902x_read(sii902x->client, 0x1A);
+	} while ((!(dat & 0x2)) && cnt);
+
+	if (!cnt) {
+		ret = -1;
+		goto done;
+	}
+
+	sii902x_write(sii902x->client, 0x1A, old | 0x06);
+
+	/* edid reading */
+	ret = __sii902x_get_edid(sii902x->client->adapter, fbi);
+
+	cnt = 100;
+	do {
+		cnt--;
+		sii902x_write(sii902x->client, 0x1A, old & ~0x6);
+		msleep(20);
+		dat = sii902x_read(sii902x->client, 0x1A);
+	} while ((dat & 0x6) && cnt);
+
+	if (!cnt)
+		ret = -1;
+
+done:
+	sii902x_write(sii902x->client, 0x1A, old);
+	return ret;
+}
+
+static void det_worker(struct work_struct *work)
+{
+	struct fb_info *fbi = sii902x->fbi;
+	struct fb_monspecs *monspecs = &fbi->monspecs;
+	int val, ret;
+	char event_string[16];
+	char *envp[] = { event_string, NULL };
+
+	val = sii902x_read(sii902x->client, 0x3D);
+	if (!(val & 0x1) && !g_enable_hdmi)
+		goto err;
+
+	/* cable connection changes */
+	if (val & 0x4 || g_enable_hdmi) {
+		sii902x->cable_plugin = 1;
+		sprintf(event_string, "EVENT=plugin");
+
+		ret = sii902x_get_edid(fbi);
+		if (ret < 0) {
+			dev_err(&sii902x->client->dev, "read edid fail\n");
+			goto err;
+		}
+
+		/* make sure fb is powerdown */
+		console_lock();
+		fb_blank(fbi, FB_BLANK_POWERDOWN);
+		console_unlock();
+
+		if (monspecs->modedb_len > 0) {
+			int i;
+			const struct fb_videomode *mode;
+			struct fb_videomode m;
+
+			fb_destroy_modelist(&fbi->modelist);
+
+			for (i = 0; i < monspecs->modedb_len; i++) {
+				/* We do not support interlaced mode for now */
+				if (!(monspecs->modedb[i].vmode &
+					FB_VMODE_INTERLACED))
+					fb_add_videomode(&monspecs->modedb[i],
+							&fbi->modelist);
+			}
+
+			fb_var_to_videomode(&m, &fbi->var);
+			mode = fb_find_nearest_mode(&m,
+					&fbi->modelist);
+
+			fb_videomode_to_var(&fbi->var, mode);
+
+			fbi->var.activate |= FB_ACTIVATE_FORCE;
+			console_lock();
+			fbi->flags |= FBINFO_MISC_USEREVENT;
+			fb_set_var(fbi, &fbi->var);
+			fbi->flags &= ~FBINFO_MISC_USEREVENT;
+			console_unlock();
+		}
+
+		console_lock();
+		fb_blank(fbi, FB_BLANK_UNBLANK);
+		console_unlock();
+	} else {
+		sii902x->cable_plugin = 0;
+		sprintf(event_string, "EVENT=plugout");
+		console_lock();
+		fb_blank(fbi, FB_BLANK_POWERDOWN);
+		console_unlock();
+	}
+	kobject_uevent_env(&sii902x->client->dev.kobj,
+			KOBJ_CHANGE, envp);
+
+err:
+	sii902x_write(sii902x->client, 0x3D, val);
+}
+
+static irqreturn_t sii902x_detect_handler(int irq, void *data)
+{
+	if (g_enable_hdmi)
+		g_enable_hdmi = false;
+
+	if (sii902x->fbi)
+		schedule_delayed_work(&(sii902x->det_work),
+				msecs_to_jiffies(20));
+	return IRQ_HANDLED;
+}
+
+static void sii902x_poweron(void)
+{
+	/* Turn on DVI or HDMI */
+	sii902x_write(sii902x->client, 0x1A, 0x00);
+}
+
+static void sii902x_poweroff(void)
+{
+	/* disable tmds before changing resolution */
+	sii902x_write(sii902x->client, 0x1A, 0x10);
+}
+
+static int sii902x_fb_event(struct notifier_block *nb,
+			    unsigned long val, void *v)
+{
+	struct fb_event *event = v;
+	struct fb_info *fbi = event->info;
+
+	switch (val) {
+	case FB_EVENT_FB_REGISTERED:
+		if (sii902x->fbi != NULL)
+			break;
+		sii902x->fbi = fbi;
+		if (g_enable_hdmi && sii902x->fbi) {
+			schedule_delayed_work(&(sii902x->det_work),
+					msecs_to_jiffies(20));
+		}
+		break;
+	case FB_EVENT_MODE_CHANGE:
+		sii902x_setup(fbi);
+		break;
+	case FB_EVENT_BLANK:
+		if (*((int *)event->data) == FB_BLANK_UNBLANK)
+			sii902x_poweron();
+		else
+			sii902x_poweroff();
+		break;
+	}
+
+	return 0;
+}
+
+static void sii902x_chip_id(struct sii902x_data *sii902x)
+{
+	struct i2c_client *client = sii902x_to_i2c(sii902x);
+	int val;
+
+	/* read device ID */
+	val = sii902x_read(client, 0x1B);
+	printk(KERN_INFO "Sii902x: read id = 0x%02X", val);
+	val = sii902x_read(client, 0x1C);
+	printk(KERN_INFO "-0x%02X", val);
+	val = sii902x_read(client, 0x1D);
+	printk(KERN_INFO "-0x%02X", val);
+	val = sii902x_read(client, 0x30);
+	printk(KERN_INFO "-0x%02X\n", val);
+}
+
+static int sii902x_initialize(struct sii902x_data *sii902x)
+{
+	struct i2c_client *client = sii902x_to_i2c(sii902x);
+	int ret, cnt;
+
+	for (cnt = 0; cnt < 5; cnt++) {
+		/* Set 902x in hardware TPI mode on and jump out of D3 state */
+		ret = sii902x_write(client, 0xC7, 0x00);
+		if (ret < 0)
+			break;
+	}
+	if (0 != ret)
+		dev_err(&client->dev, "cound not find device\n");
+
+	return ret;
+}
+
+static void sii902x_enable_source(struct sii902x_data *sii902x)
+{
+	struct i2c_client *client = sii902x_to_i2c(sii902x);
+	int val;
+
+	sii902x_write(client, 0xBC, 0x01);
+	sii902x_write(client, 0xBD, 0x82);
+	val = sii902x_read(client, 0xBE);
+	val |= 0x1;
+	sii902x_write(client, 0xBE, val);
+}
+
+static struct notifier_block nb = {
+	.notifier_call = sii902x_fb_event,
+};
+
+static int sii902x_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = to_i2c_adapter(client->dev.parent);
+	int ret, err;
+	struct fb_info edid_fbi;
+
+	if (!g_enable_hdmi)
+		return -EPERM;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE)) {
+		dev_err(&client->dev, "i2c_check_functionality error\n");
+		return -ENODEV;
+	}
+
+	sii902x = devm_kzalloc(&client->dev, sizeof(*sii902x), GFP_KERNEL);
+	if (!sii902x)
+		return -ENOMEM;
+
+	sii902x->client = client;
+	i2c_set_clientdata(client, sii902x);
+
+	err = sii902x_initialize(sii902x);
+	if (err)
+		return err;
+
+	sii902x_chip_id(sii902x);
+	sii902x_power_up_tx(sii902x);
+	sii902x_enable_source(sii902x);
+
+	/* try to read edid */
+	if (sii902x_get_edid(&edid_fbi) < 0)
+		dev_warn(&client->dev, "Can not read edid\n");
+
+	if (client->irq) {
+		ret = devm_request_irq(&client->dev, client->irq,
+				sii902x_detect_handler, 0,
+				"SII902x_det", sii902x);
+		if (ret < 0)
+			dev_warn(&client->dev,
+				"cound not request det irq %d\n",
+				client->irq);
+		else {
+			INIT_DELAYED_WORK(&(sii902x->det_work), det_worker);
+			/*enable cable hot plug irq*/
+			sii902x_write(client, 0x3C, 0x01);
+		}
+		ret = device_create_file(&client->dev, &dev_attr_fb_name);
+		if (ret < 0)
+			dev_warn(&client->dev,
+				"cound not create sys node for fb name\n");
+		ret = device_create_file(&client->dev, &dev_attr_cable_state);
+		if (ret < 0)
+			dev_warn(&client->dev,
+				"cound not create sys node for cable state\n");
+	}
+
+	fb_register_client(&nb);
+
+	return 0;
+}
+
+static int sii902x_remove(struct i2c_client *client)
+{
+	fb_unregister_client(&nb);
+	sii902x_poweroff();
+	return 0;
+}
+
+static const struct i2c_device_id sii902x_id[] = {
+	{ "sii902x", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, sii902x_id);
+
+static const struct of_device_id sii902x_dt_ids[] = {
+	{ .compatible = "fsl,sii902x", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
+
+static struct i2c_driver sii902x_i2c_driver = {
+	.driver = {
+		.name = "sii902x",
+		.owner = THIS_MODULE,
+		.of_match_table = sii902x_dt_ids,
+	},
+	.probe = sii902x_probe,
+	.remove = sii902x_remove,
+	.id_table = sii902x_id,
+};
+module_i2c_driver(sii902x_i2c_driver);
+
+static int __init enable_hdmi_setup(char *str)
+{
+	g_enable_hdmi = true;
+
+	return 1;
+}
+__setup("hdmi", enable_hdmi_setup);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("SII902x DVI/HDMI driver");
+MODULE_LICENSE("GPL");
-- 
2.1.0.27.g96db324

Re: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[Tomi Valkeinen]

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

Hi,

On 05/09/14 07:48, Xiubo Li wrote:
> Some Freescale SoCs, there has an DVI/HDMI controller and a PHY,
> attached to one of their display controller unit's LCDC interfaces.
> This patch adds a preliminary static support for such controllers.
> 
> This will support for many modes and a dynamic switching between
> them.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  .../devicetree/bindings/video/fsl-sii902x.txt      |  17 +
>  drivers/video/fbdev/Kconfig                        |   7 +
>  drivers/video/fbdev/Makefile                       |   1 +
>  drivers/video/fbdev/fsl-sii902x.c                  | 526 +++++++++++++++++++++
>  4 files changed, 551 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/fsl-sii902x.txt
>  create mode 100644 drivers/video/fbdev/fsl-sii902x.c

I don't know how you picked the names of the people you sent this patch
to, but looks to me that most of them are probably not interested in
this patch.

Anyway, a few quick comments on the patch:

- You should probably use regmap instead of direct i2c calls.
Interestingly, you define regmap variable, but you never use it.
- Use defines for register offsets, instead of magic numbers.
- You should not use static variables. They prevent having multiple
instances of the device.

So the SiI902x chip is on the SoC, not on the board? And it's a plain
standard SiI902x in other aspects?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[Li.Xiubo]

Hi Tomi,

Thanks very much for your comments.


> Subject: Re: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs
> 
> Hi,
> 
> On 05/09/14 07:48, Xiubo Li wrote:
> > Some Freescale SoCs, there has an DVI/HDMI controller and a PHY,
> > attached to one of their display controller unit's LCDC interfaces.
> > This patch adds a preliminary static support for such controllers.
> >
> > This will support for many modes and a dynamic switching between
> > them.
> >
> > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> > ---
> >  .../devicetree/bindings/video/fsl-sii902x.txt      |  17 +
> >  drivers/video/fbdev/Kconfig                        |   7 +
> >  drivers/video/fbdev/Makefile                       |   1 +
> >  drivers/video/fbdev/fsl-sii902x.c                  | 526
> +++++++++++++++++++++
> >  4 files changed, 551 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/fsl-sii902x.txt
> >  create mode 100644 drivers/video/fbdev/fsl-sii902x.c
> 
> I don't know how you picked the names of the people you sent this patch
> to, but looks to me that most of them are probably not interested in
> this patch.
> 

I just using the get_maintainer.pl script.


> Anyway, a few quick comments on the patch:
> 
> - You should probably use regmap instead of direct i2c calls.
> Interestingly, you define regmap variable, but you never use it.

Yes, this it's my another version in my local machine using regmap which
need much more test.

> - Use defines for register offsets, instead of magic numbers.

This will be fixed together with regmap using.

> - You should not use static variables. They prevent having multiple
> instances of the device.
> 

Okay.

> So the SiI902x chip is on the SoC, not on the board? And it's a plain
> standard SiI902x in other aspects?
> 

No, it's on board.

And it will be used for i.MX and LS1+ platforms.

Thanks,

BRs
Xiubo

Re: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[Tomi Valkeinen]

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

On 15/09/14 05:17, Li.Xiubo@freescale.com wrote:
> Hi Tomi,
> 
> Thanks very much for your comments.
> 
> 
>> Subject: Re: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs
>>
>> Hi,
>>
>> On 05/09/14 07:48, Xiubo Li wrote:
>>> Some Freescale SoCs, there has an DVI/HDMI controller and a PHY,
>>> attached to one of their display controller unit's LCDC interfaces.
>>> This patch adds a preliminary static support for such controllers.
>>>
>>> This will support for many modes and a dynamic switching between
>>> them.
>>>
>>> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
>>> ---
>>>  .../devicetree/bindings/video/fsl-sii902x.txt      |  17 +
>>>  drivers/video/fbdev/Kconfig                        |   7 +
>>>  drivers/video/fbdev/Makefile                       |   1 +
>>>  drivers/video/fbdev/fsl-sii902x.c                  | 526
>> +++++++++++++++++++++
>>>  4 files changed, 551 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/video/fsl-sii902x.txt
>>>  create mode 100644 drivers/video/fbdev/fsl-sii902x.c
>>
>> I don't know how you picked the names of the people you sent this patch
>> to, but looks to me that most of them are probably not interested in
>> this patch.
>>
> 
> I just using the get_maintainer.pl script.

Yes, and that's good, but you have to use your judgment also.
get_maintainer.pl gives you names of people who have at some point
touched the files or directories you are changing. Usually only the
first names returned by get_maintainer.pl are relevant.

>> Anyway, a few quick comments on the patch:
>>
>> - You should probably use regmap instead of direct i2c calls.
>> Interestingly, you define regmap variable, but you never use it.
> 
> Yes, this it's my another version in my local machine using regmap which
> need much more test.
> 
>> - Use defines for register offsets, instead of magic numbers.
> 
> This will be fixed together with regmap using.

Well, it's better to send the patch only when you have tested and
cleaned up your driver.

>> - You should not use static variables. They prevent having multiple
>> instances of the device.
>>
> 
> Okay.
> 
>> So the SiI902x chip is on the SoC, not on the board? And it's a plain
>> standard SiI902x in other aspects?
>>
> 
> No, it's on board.
> 
> And it will be used for i.MX and LS1+ platforms.

Ok. In that case, I would suggest you to move to the DRM framework. The
fbdev framework is not suited to adding external encoders. The end
result with fbdev will be a SoC/board specific hack driver.

That said, we already have such drivers for fbdev, so I'm not 100%
against adding a new one. But I'm very very seriously recommending
moving to DRM.

And if this driver is added to fbdev, I think the device tree bindings
should use the video ports/endpoints to describe the connections.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] fbdev: fsl-sii902x: HDMI support for Freescale SoCs

[Li.Xiubo]

Hi,


[...]
> >>>  4 files changed, 551 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/video/fsl-
> sii902x.txt
> >>>  create mode 100644 drivers/video/fbdev/fsl-sii902x.c
> >>
> >> I don't know how you picked the names of the people you sent this patch
> >> to, but looks to me that most of them are probably not interested in
> >> this patch.
> >>
> >
> > I just using the get_maintainer.pl script.
> 
> Yes, and that's good, but you have to use your judgment also.
> get_maintainer.pl gives you names of people who have at some point
> touched the files or directories you are changing. Usually only the
> first names returned by get_maintainer.pl are relevant.
> 
Okay :)


> >> Anyway, a few quick comments on the patch:
> >>
> >> - You should probably use regmap instead of direct i2c calls.
> >> Interestingly, you define regmap variable, but you never use it.
> >
> > Yes, this it's my another version in my local machine using regmap which
> > need much more test.
> >
> >> - Use defines for register offsets, instead of magic numbers.
> >
> > This will be fixed together with regmap using.
> 
> Well, it's better to send the patch only when you have tested and
> cleaned up your driver.
> 
> >> - You should not use static variables. They prevent having multiple
> >> instances of the device.
> >>
> >
> > Okay.
> >
> >> So the SiI902x chip is on the SoC, not on the board? And it's a plain
> >> standard SiI902x in other aspects?
> >>
> >
> > No, it's on board.
> >
> > And it will be used for i.MX and LS1+ platforms.
> 
> Ok. In that case, I would suggest you to move to the DRM framework. The
> fbdev framework is not suited to adding external encoders. The end
> result with fbdev will be a SoC/board specific hack driver.
> 
> That said, we already have such drivers for fbdev, so I'm not 100%
> against adding a new one. But I'm very very seriously recommending
> moving to DRM.
> 
> And if this driver is added to fbdev, I think the device tree bindings
> should use the video ports/endpoints to describe the connections.
>
I will have a try to use the DRM framework.

Thanks,

BRs
Xiubo
 
>  Tomi
>