On 15/09/14 05:17, Li.Xiubo@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx> >>> --- >>> .../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:
signature.asc
Description: OpenPGP digital signature
