On 15/09/14 05:17, Li.Xiubo@freescale.com wrote:

Hi Tomi,

Thanks very much for your comments.

quoted

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

Hi,

On 05/09/14 07:48, Xiubo Li wrote:

quoted

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 <redacted>
---
 .../devicetree/bindings/video/fsl-sii902x.txt      |  17 +
 drivers/video/fbdev/Kconfig                        |   7 +
 drivers/video/fbdev/Makefile                       |   1 +
 drivers/video/fbdev/fsl-sii902x.c                  | 526

+++++++++++++++++++++

quoted

 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.

quoted

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.

quoted

- 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.

quoted

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

Okay.

quoted

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